From 5831d4f89a2a045e821943ccfc0c02f690c407fa Mon Sep 17 00:00:00 2001 From: Ryan Rossiter Date: Wed, 6 Jan 2016 22:50:54 +0000 Subject: [PATCH] Remove NovaObjectDictCompat from Aggregate This change removes the NovaObjectDictCompat mixin from the Aggregate versioned object. All occurrences of key access or get have been changed to use getattr/setattr. Because none of the objects externally-facing functionality has changed, the object version does not need to be updated. There was a fake aggregate in the xen api tests that contained two empty dictionaries for a couple of keys in the metadata. These are actually strings in the code, so they have been changed to empty strings. Within the tests for the aggregate API, the fake dictionaries that were returned by the AggregateAPI stubs were structured incorrectly. Within the object, the availability zone is stored in the metadata dictionary, not the top-level dictionary. Also, those dictionaries were changed to Aggregate objects, so the stubs now return true objects (similar to how the actual code works). The result of the calls are then compared by the object primitives. There was usage of .items() in _marshall_aggregate() within the api controller for aggregates, so a unit test was added to run through that function to make sure it is transforming the aggregate object correctly. (props to Tempest for finding that one for me). Partially-Implements: bp rm-object-dict-compat Change-Id: Ibcb896b2eef673bce23491161ff394bcc87aa541 --- nova/api/openstack/compute/aggregates.py | 9 +- .../compute/legacy_v2/contrib/aggregates.py | 9 +- nova/compute/api.py | 2 +- nova/objects/aggregate.py | 6 +- .../api/openstack/compute/test_aggregates.py | 111 ++++++++++++--- nova/tests/unit/compute/test_compute.py | 130 +++++++++--------- nova/tests/unit/virt/xenapi/test_xenapi.py | 19 +-- nova/virt/xenapi/pool.py | 44 +++--- 8 files changed, 205 insertions(+), 125 deletions(-) diff --git a/nova/api/openstack/compute/aggregates.py b/nova/api/openstack/compute/aggregates.py index d9f46279e18d..5dd4067df680 100644 --- a/nova/api/openstack/compute/aggregates.py +++ b/nova/api/openstack/compute/aggregates.py @@ -199,13 +199,20 @@ class AggregateController(wsgi.Controller): def _marshall_aggregate(self, aggregate): _aggregate = {} - for key, value in aggregate.items(): + for key, value in self._build_aggregate_items(aggregate): # NOTE(danms): The original API specified non-TZ-aware timestamps if isinstance(value, datetime.datetime): value = value.replace(tzinfo=None) _aggregate[key] = value return {"aggregate": _aggregate} + def _build_aggregate_items(self, aggregate): + keys = aggregate.obj_fields + for key in keys: + if (aggregate.obj_attr_is_set(key) + or key in aggregate.obj_extra_fields): + yield key, getattr(aggregate, key) + class Aggregates(extensions.V21APIExtensionBase): """Admin-only aggregate administration.""" diff --git a/nova/api/openstack/compute/legacy_v2/contrib/aggregates.py b/nova/api/openstack/compute/legacy_v2/contrib/aggregates.py index 4a289a4e2918..6e0a0a31fe1f 100644 --- a/nova/api/openstack/compute/legacy_v2/contrib/aggregates.py +++ b/nova/api/openstack/compute/legacy_v2/contrib/aggregates.py @@ -268,13 +268,20 @@ class AggregateController(object): def _marshall_aggregate(self, aggregate): _aggregate = {} - for key, value in aggregate.items(): + for key, value in self._build_aggregate_items(aggregate): # NOTE(danms): The original API specified non-TZ-aware timestamps if isinstance(value, datetime.datetime): value = value.replace(tzinfo=None) _aggregate[key] = value return {"aggregate": _aggregate} + def _build_aggregate_items(self, aggregate): + keys = aggregate.obj_fields + for key in keys: + if (aggregate.obj_attr_is_set(key) + or key in aggregate.obj_extra_fields): + yield key, getattr(aggregate, key) + class Aggregates(extensions.ExtensionDescriptor): """Admin-only aggregate administration.""" diff --git a/nova/compute/api.py b/nova/compute/api.py index 26fcb703b142..2794130e57d3 100644 --- a/nova/compute/api.py +++ b/nova/compute/api.py @@ -3630,7 +3630,7 @@ class AggregateAPI(base.Base): # NOTE(jogo): Send message to host to support resource pools self.compute_rpcapi.add_aggregate_host(context, aggregate=aggregate, host_param=host_name, host=host_name) - aggregate_payload.update({'name': aggregate['name']}) + aggregate_payload.update({'name': aggregate.name}) compute_utils.notify_about_aggregate_update(context, "addhost.end", aggregate_payload) diff --git a/nova/objects/aggregate.py b/nova/objects/aggregate.py index 604bed05cdac..6510567b257e 100644 --- a/nova/objects/aggregate.py +++ b/nova/objects/aggregate.py @@ -20,10 +20,8 @@ from nova.objects import base from nova.objects import fields -# TODO(berrange): Remove NovaObjectDictCompat @base.NovaObjectRegistry.register -class Aggregate(base.NovaPersistentObject, base.NovaObject, - base.NovaObjectDictCompat): +class Aggregate(base.NovaPersistentObject, base.NovaObject): # Version 1.0: Initial version # Version 1.1: String attributes updated to support unicode VERSION = '1.1' @@ -44,7 +42,7 @@ class Aggregate(base.NovaPersistentObject, base.NovaObject, db_key = 'metadetails' else: db_key = key - aggregate[key] = db_aggregate[db_key] + setattr(aggregate, key, db_aggregate[db_key]) aggregate._context = context aggregate.obj_reset_changes() return aggregate diff --git a/nova/tests/unit/api/openstack/compute/test_aggregates.py b/nova/tests/unit/api/openstack/compute/test_aggregates.py index ee1d210787a6..d49cdb8f4016 100644 --- a/nova/tests/unit/api/openstack/compute/test_aggregates.py +++ b/nova/tests/unit/api/openstack/compute/test_aggregates.py @@ -24,24 +24,60 @@ from nova.api.openstack.compute.legacy_v2.contrib import aggregates \ from nova.compute import api as compute_api from nova import context from nova import exception +from nova import objects from nova import test from nova.tests.unit.api.openstack import fakes from nova.tests.unit import matchers + +def _make_agg_obj(agg_dict): + return objects.Aggregate(**agg_dict) + + +def _make_agg_list(agg_list): + return objects.AggregateList(objects=[_make_agg_obj(a) for a in agg_list]) + + +def _transform_aggregate_az(agg_dict): + # the Aggregate object looks for availability_zone within metadata, + # so if availability_zone is in the top-level dict, move it down into + # metadata. We also have to delete the key from the top-level dict because + # availability_zone is a read-only property on the Aggregate object + md = agg_dict.get('metadata', {}) + if 'availability_zone' in agg_dict: + md['availability_zone'] = agg_dict['availability_zone'] + del agg_dict['availability_zone'] + agg_dict['metadata'] = md + return agg_dict + + +def _transform_aggregate_list_azs(agg_list): + for agg_dict in agg_list: + yield _transform_aggregate_az(agg_dict) + + AGGREGATE_LIST = [ - {"name": "aggregate1", "id": "1", "availability_zone": "nova1"}, - {"name": "aggregate2", "id": "2", "availability_zone": "nova1"}, - {"name": "aggregate3", "id": "3", "availability_zone": "nova2"}, - {"name": "aggregate1", "id": "4", "availability_zone": "nova1"}] + {"name": "aggregate1", "id": "1", + "metadata": {"availability_zone": "nova1"}}, + {"name": "aggregate2", "id": "2", + "metadata": {"availability_zone": "nova1"}}, + {"name": "aggregate3", "id": "3", + "metadata": {"availability_zone": "nova2"}}, + {"name": "aggregate1", "id": "4", + "metadata": {"availability_zone": "nova1"}}] +AGGREGATE_LIST = _make_agg_list(AGGREGATE_LIST) + AGGREGATE = {"name": "aggregate1", "id": "1", - "availability_zone": "nova1", - "metadata": {"foo": "bar"}, - "hosts": ["host1, host2"]} + "metadata": {"foo": "bar", + "availability_zone": "nova1"}, + "hosts": ["host1", "host2"]} +AGGREGATE = _make_agg_obj(AGGREGATE) FORMATTED_AGGREGATE = {"name": "aggregate1", "id": "1", - "availability_zone": "nova1"} + "metadata": {"availability_zone": "nova1"}} +FORMATTED_AGGREGATE = _make_agg_obj(FORMATTED_AGGREGATE) class FakeRequest(object): @@ -76,8 +112,8 @@ class AggregateTestCaseV21(test.NoDBTestCase): stub_list_aggregates) result = self.controller.index(self.req) - - self.assertEqual(AGGREGATE_LIST, result["aggregates"]) + result = _transform_aggregate_list_azs(result['aggregates']) + self._assert_agg_data(AGGREGATE_LIST, _make_agg_list(result)) def test_index_no_admin(self): self.assertRaises(exception.PolicyNotAuthorized, @@ -96,7 +132,8 @@ class AggregateTestCaseV21(test.NoDBTestCase): result = self.controller.create(self.req, body={"aggregate": {"name": "test", "availability_zone": "nova1"}}) - self.assertEqual(FORMATTED_AGGREGATE, result["aggregate"]) + result = _transform_aggregate_az(result['aggregate']) + self._assert_agg_data(FORMATTED_AGGREGATE, _make_agg_obj(result)) def test_create_no_admin(self): self.assertRaises(exception.PolicyNotAuthorized, @@ -175,7 +212,9 @@ class AggregateTestCaseV21(test.NoDBTestCase): result = self.controller.create(self.req, body={"aggregate": {"name": "test"}}) - self.assertEqual(FORMATTED_AGGREGATE, result["aggregate"]) + + result = _transform_aggregate_az(result['aggregate']) + self._assert_agg_data(FORMATTED_AGGREGATE, _make_agg_obj(result)) def test_create_with_null_name(self): self.assertRaises(self.bad_request, self.controller.create, @@ -238,8 +277,8 @@ class AggregateTestCaseV21(test.NoDBTestCase): stub_get_aggregate) aggregate = self.controller.show(self.req, "1") - - self.assertEqual(AGGREGATE, aggregate["aggregate"]) + aggregate = _transform_aggregate_az(aggregate['aggregate']) + self._assert_agg_data(AGGREGATE, _make_agg_obj(aggregate)) def test_show_no_admin(self): self.assertRaises(exception.PolicyNotAuthorized, @@ -270,7 +309,8 @@ class AggregateTestCaseV21(test.NoDBTestCase): result = self.controller.update(self.req, "1", body=body) - self.assertEqual(AGGREGATE, result["aggregate"]) + result = _transform_aggregate_az(result['aggregate']) + self._assert_agg_data(AGGREGATE, _make_agg_obj(result)) def test_update_no_admin(self): body = {"aggregate": {"availability_zone": "nova"}} @@ -288,7 +328,8 @@ class AggregateTestCaseV21(test.NoDBTestCase): result = self.controller.update(self.req, "1", body=body) - self.assertEqual(AGGREGATE, result["aggregate"]) + result = _transform_aggregate_az(result['aggregate']) + self._assert_agg_data(AGGREGATE, _make_agg_obj(result)) def test_update_with_only_availability_zone(self): body = {"aggregate": {"availability_zone": "nova1"}} @@ -298,7 +339,9 @@ class AggregateTestCaseV21(test.NoDBTestCase): self.stubs.Set(self.controller.api, "update_aggregate", stub_update_aggregate) result = self.controller.update(self.req, "1", body=body) - self.assertEqual(AGGREGATE, result["aggregate"]) + + result = _transform_aggregate_az(result['aggregate']) + self._assert_agg_data(AGGREGATE, _make_agg_obj(result)) def test_update_with_no_updates(self): test_metadata = {"aggregate": {}} @@ -384,7 +427,8 @@ class AggregateTestCaseV21(test.NoDBTestCase): body={"add_host": {"host": "host1"}}) - self.assertEqual(aggregate["aggregate"], AGGREGATE) + aggregate = _transform_aggregate_az(aggregate['aggregate']) + self._assert_agg_data(AGGREGATE, aggregate) def test_add_host_no_admin(self): self.assertRaises(exception.PolicyNotAuthorized, @@ -458,7 +502,8 @@ class AggregateTestCaseV21(test.NoDBTestCase): self.assertEqual("1", aggregate, "aggregate") self.assertEqual("host1", host, "host") stub_remove_host_from_aggregate.called = True - return {} + # at minimum, metadata is always set to {} in the api + return _make_agg_obj({'metadata': {}}) self.stubs.Set(self.controller.api, "remove_host_from_aggregate", stub_remove_host_from_aggregate) @@ -545,7 +590,8 @@ class AggregateTestCaseV21(test.NoDBTestCase): result = eval(self.set_metadata)(self.req, "1", body=body) - self.assertEqual(AGGREGATE, result["aggregate"]) + result = _transform_aggregate_az(result['aggregate']) + self._assert_agg_data(AGGREGATE, _make_agg_obj(result)) def test_set_metadata_delete(self): body = {"set_metadata": {"metadata": {"foo": None}}} @@ -555,7 +601,8 @@ class AggregateTestCaseV21(test.NoDBTestCase): mocked.return_value = AGGREGATE result = eval(self.set_metadata)(self.req, "1", body=body) - self.assertEqual(AGGREGATE, result["aggregate"]) + result = _transform_aggregate_az(result['aggregate']) + self._assert_agg_data(AGGREGATE, _make_agg_obj(result)) mocked.assert_called_once_with(self.context, "1", body["set_metadata"]["metadata"]) @@ -646,6 +693,28 @@ class AggregateTestCaseV21(test.NoDBTestCase): self.controller.delete, self.req, "agg1") + def test_marshall_aggregate(self): + # _marshall_aggregate() just basically turns the aggregate returned + # from the AggregateAPI into a dict, so this tests that transform. + # We would expect the dictionary that comes out is the same one + # that we pump into the aggregate object in the first place + agg = {'name': 'aggregate1', + 'id': 1, + 'metadata': {'foo': 'bar', 'availability_zone': 'nova'}, + 'hosts': ['host1', 'host2']} + agg_obj = _make_agg_obj(agg) + marshalled_agg = self.controller._marshall_aggregate(agg_obj) + + # _marshall_aggregate() puts all fields and obj_extra_fields in the + # top-level dict, so we need to put availability_zone at the top also + agg['availability_zone'] = 'nova' + self.assertEqual(agg, marshalled_agg['aggregate']) + + def _assert_agg_data(self, expected, actual): + expected_data = expected.obj_to_primitive()['nova_object.data'] + actual_data = expected.obj_to_primitive()['nova_object.data'] + self.assertEqual(expected_data, actual_data) + class AggregateTestCaseV2(AggregateTestCaseV21): add_host = 'self.controller.action' diff --git a/nova/tests/unit/compute/test_compute.py b/nova/tests/unit/compute/test_compute.py index dd1410bdf131..1350e24c6f92 100644 --- a/nova/tests/unit/compute/test_compute.py +++ b/nova/tests/unit/compute/test_compute.py @@ -344,7 +344,7 @@ class BaseTestCase(test.TestCase): def _init_aggregate_with_host(self, aggr, aggr_name, zone, host): if not aggr: aggr = self.api.create_aggregate(self.context, aggr_name, zone) - aggr = self.api.add_host_to_aggregate(self.context, aggr['id'], host) + aggr = self.api.add_host_to_aggregate(self.context, aggr.id, host) return aggr @@ -10247,11 +10247,11 @@ class ComputeAPIAggrTestCase(BaseTestCase): # Ensure we can create an aggregate without an availability zone aggr = self.api.create_aggregate(self.context, 'fake_aggregate', None) - self.api.delete_aggregate(self.context, aggr['id']) + self.api.delete_aggregate(self.context, aggr.id) db.aggregate_get(self.context.elevated(read_deleted='yes'), - aggr['id']) + aggr.id) self.assertRaises(exception.AggregateNotFound, - self.api.delete_aggregate, self.context, aggr['id']) + self.api.delete_aggregate, self.context, aggr.id) def test_check_az_for_aggregate(self): # Ensure all conflict hosts can be returned @@ -10268,14 +10268,14 @@ class ComputeAPIAggrTestCase(BaseTestCase): metadata = {'availability_zone': 'another_zone'} self.assertRaises(exception.InvalidAggregateActionUpdate, self.api.update_aggregate, - self.context, aggr2['id'], metadata) + self.context, aggr2.id, metadata) def test_update_aggregate(self): # Ensure metadata can be updated. aggr = self.api.create_aggregate(self.context, 'fake_aggregate', 'fake_zone') fake_notifier.NOTIFICATIONS = [] - self.api.update_aggregate(self.context, aggr['id'], + self.api.update_aggregate(self.context, aggr.id, {'name': 'new_fake_aggregate'}) self.assertIsNone(availability_zones._get_cache().get('cache')) self.assertEqual(len(fake_notifier.NOTIFICATIONS), 2) @@ -10299,7 +10299,7 @@ class ComputeAPIAggrTestCase(BaseTestCase): fake_host) metadata = {'name': 'new_fake_aggregate'} fake_notifier.NOTIFICATIONS = [] - self.api.update_aggregate(self.context, aggr2['id'], metadata) + self.api.update_aggregate(self.context, aggr2.id, metadata) self.assertEqual(len(fake_notifier.NOTIFICATIONS), 2) msg = fake_notifier.NOTIFICATIONS[0] self.assertEqual(msg.event_type, @@ -10321,7 +10321,7 @@ class ComputeAPIAggrTestCase(BaseTestCase): fake_host) metadata = {'availability_zone': 'new_fake_zone'} fake_notifier.NOTIFICATIONS = [] - self.api.update_aggregate(self.context, aggr1['id'], metadata) + self.api.update_aggregate(self.context, aggr1.id, metadata) self.assertEqual(len(fake_notifier.NOTIFICATIONS), 2) msg = fake_notifier.NOTIFICATIONS[0] self.assertEqual(msg.event_type, @@ -10344,12 +10344,12 @@ class ComputeAPIAggrTestCase(BaseTestCase): metadata = {'availability_zone': 'another_zone'} self.assertRaises(exception.InvalidAggregateActionUpdate, self.api.update_aggregate, - self.context, aggr2['id'], metadata) + self.context, aggr2.id, metadata) fake_host2 = values[0][1][1] aggr3 = self._init_aggregate_with_host(None, 'fake_aggregate3', None, fake_host2) metadata = {'availability_zone': fake_zone} - self.api.update_aggregate(self.context, aggr3['id'], metadata) + self.api.update_aggregate(self.context, aggr3.id, metadata) self.assertEqual(len(fake_notifier.NOTIFICATIONS), 15) msg = fake_notifier.NOTIFICATIONS[13] self.assertEqual(msg.event_type, @@ -10372,7 +10372,7 @@ class ComputeAPIAggrTestCase(BaseTestCase): metadata = {'availability_zone': 'another_zone'} self.assertRaises(exception.InvalidAggregateActionUpdate, self.api.update_aggregate, - self.context, aggr2['id'], metadata) + self.context, aggr2.id, metadata) def test_update_aggregate_metadata(self): # Ensure metadata can be updated. @@ -10383,7 +10383,7 @@ class ComputeAPIAggrTestCase(BaseTestCase): 'availability_zone': 'fake_zone'} fake_notifier.NOTIFICATIONS = [] availability_zones._get_cache().add('fake_key', 'fake_value') - aggr = self.api.update_aggregate_metadata(self.context, aggr['id'], + aggr = self.api.update_aggregate_metadata(self.context, aggr.id, metadata) self.assertIsNone(availability_zones._get_cache().get('fake_key')) self.assertEqual(len(fake_notifier.NOTIFICATIONS), 2) @@ -10399,7 +10399,7 @@ class ComputeAPIAggrTestCase(BaseTestCase): 'foo_key2': 'foo_value2', 'availability_zone': 'fake_zone'} expected = self.api.update_aggregate_metadata(self.context, - aggr['id'], metadata) + aggr.id, metadata) self.assertEqual(2, len(fake_notifier.NOTIFICATIONS)) msg = fake_notifier.NOTIFICATIONS[0] self.assertEqual('aggregate.updatemetadata.start', msg.event_type) @@ -10407,7 +10407,7 @@ class ComputeAPIAggrTestCase(BaseTestCase): msg = fake_notifier.NOTIFICATIONS[1] self.assertEqual('aggregate.updatemetadata.end', msg.event_type) self.assertEqual(expected_payload_meta_data, msg.payload['meta_data']) - self.assertThat(expected['metadata'], + self.assertThat(expected.metadata, matchers.DictMatches({'availability_zone': 'fake_zone', 'foo_key2': 'foo_value2'})) @@ -10424,7 +10424,7 @@ class ComputeAPIAggrTestCase(BaseTestCase): fake_host) metadata = {'foo_key2': 'foo_value3'} fake_notifier.NOTIFICATIONS = [] - aggr2 = self.api.update_aggregate_metadata(self.context, aggr2['id'], + aggr2 = self.api.update_aggregate_metadata(self.context, aggr2.id, metadata) self.assertEqual(len(fake_notifier.NOTIFICATIONS), 2) msg = fake_notifier.NOTIFICATIONS[0] @@ -10433,7 +10433,7 @@ class ComputeAPIAggrTestCase(BaseTestCase): msg = fake_notifier.NOTIFICATIONS[1] self.assertEqual(msg.event_type, 'aggregate.updatemetadata.end') - self.assertThat(aggr2['metadata'], + self.assertThat(aggr2.metadata, matchers.DictMatches({'foo_key2': 'foo_value3'})) def test_update_aggregate_metadata_az_change(self): @@ -10449,7 +10449,7 @@ class ComputeAPIAggrTestCase(BaseTestCase): fake_host) metadata = {'availability_zone': 'new_fake_zone'} fake_notifier.NOTIFICATIONS = [] - self.api.update_aggregate_metadata(self.context, aggr1['id'], metadata) + self.api.update_aggregate_metadata(self.context, aggr1.id, metadata) self.assertEqual(len(fake_notifier.NOTIFICATIONS), 2) msg = fake_notifier.NOTIFICATIONS[0] self.assertEqual(msg.event_type, @@ -10465,13 +10465,13 @@ class ComputeAPIAggrTestCase(BaseTestCase): 'fake_zone') metadata = {'foo_key1': 'foo_value1'} aggr = self.api.update_aggregate_metadata(self.context, - aggr['id'], + aggr.id, metadata) metadata = {'availability_zone': 'new_fake_zone'} aggr = self.api.update_aggregate(self.context, - aggr['id'], + aggr.id, metadata) - self.assertThat(aggr['metadata'], matchers.DictMatches( + self.assertThat(aggr.metadata, matchers.DictMatches( {'availability_zone': 'new_fake_zone', 'foo_key1': 'foo_value1'})) def test_update_aggregate_metadata_az_fails(self): @@ -10488,11 +10488,11 @@ class ComputeAPIAggrTestCase(BaseTestCase): metadata = {'availability_zone': 'another_zone'} self.assertRaises(exception.InvalidAggregateActionUpdateMeta, self.api.update_aggregate_metadata, - self.context, aggr2['id'], metadata) + self.context, aggr2.id, metadata) aggr3 = self._init_aggregate_with_host(None, 'fake_aggregate3', None, fake_host) metadata = {'availability_zone': fake_zone} - self.api.update_aggregate_metadata(self.context, aggr3['id'], metadata) + self.api.update_aggregate_metadata(self.context, aggr3.id, metadata) self.assertEqual(len(fake_notifier.NOTIFICATIONS), 15) msg = fake_notifier.NOTIFICATIONS[13] self.assertEqual(msg.event_type, @@ -10514,7 +10514,7 @@ class ComputeAPIAggrTestCase(BaseTestCase): self.assertEqual(msg.event_type, 'aggregate.create.end') fake_notifier.NOTIFICATIONS = [] - self.api.delete_aggregate(self.context, aggr['id']) + self.api.delete_aggregate(self.context, aggr.id) self.assertEqual(len(fake_notifier.NOTIFICATIONS), 2) msg = fake_notifier.NOTIFICATIONS[0] self.assertEqual(msg.event_type, @@ -10523,9 +10523,9 @@ class ComputeAPIAggrTestCase(BaseTestCase): self.assertEqual(msg.event_type, 'aggregate.delete.end') db.aggregate_get(self.context.elevated(read_deleted='yes'), - aggr['id']) + aggr.id) self.assertRaises(exception.AggregateNotFound, - self.api.delete_aggregate, self.context, aggr['id']) + self.api.delete_aggregate, self.context, aggr.id) def test_delete_non_empty_aggregate(self): # Ensure InvalidAggregateAction is raised when non empty aggregate. @@ -10533,9 +10533,9 @@ class ComputeAPIAggrTestCase(BaseTestCase): [['fake_availability_zone', ['fake_host']]]) aggr = self.api.create_aggregate(self.context, 'fake_aggregate', 'fake_availability_zone') - self.api.add_host_to_aggregate(self.context, aggr['id'], 'fake_host') + self.api.add_host_to_aggregate(self.context, aggr.id, 'fake_host') self.assertRaises(exception.InvalidAggregateActionDelete, - self.api.delete_aggregate, self.context, aggr['id']) + self.api.delete_aggregate, self.context, aggr.id) def test_add_host_to_aggregate(self): # Ensure we can add a host to an aggregate. @@ -10546,7 +10546,7 @@ class ComputeAPIAggrTestCase(BaseTestCase): 'fake_aggregate', fake_zone) def fake_add_aggregate_host(*args, **kwargs): - hosts = kwargs["aggregate"]["hosts"] + hosts = kwargs["aggregate"].hosts self.assertIn(fake_host, hosts) self.stubs.Set(self.api.compute_rpcapi, 'add_aggregate_host', @@ -10561,7 +10561,7 @@ class ComputeAPIAggrTestCase(BaseTestCase): fake_notifier.NOTIFICATIONS = [] aggr = self.api.add_host_to_aggregate(self.context, - aggr['id'], fake_host) + aggr.id, fake_host) self.assertEqual(len(fake_notifier.NOTIFICATIONS), 2) msg = fake_notifier.NOTIFICATIONS[0] self.assertEqual(msg.event_type, @@ -10569,7 +10569,7 @@ class ComputeAPIAggrTestCase(BaseTestCase): msg = fake_notifier.NOTIFICATIONS[1] self.assertEqual(msg.event_type, 'aggregate.addhost.end') - self.assertEqual(len(aggr['hosts']), 1) + self.assertEqual(len(aggr.hosts), 1) def test_add_host_to_aggr_with_no_az(self): values = _create_service_entries(self.context) @@ -10577,15 +10577,15 @@ class ComputeAPIAggrTestCase(BaseTestCase): fake_host = values[0][1][0] aggr = self.api.create_aggregate(self.context, 'fake_aggregate', fake_zone) - aggr = self.api.add_host_to_aggregate(self.context, aggr['id'], + aggr = self.api.add_host_to_aggregate(self.context, aggr.id, fake_host) aggr_no_az = self.api.create_aggregate(self.context, 'fake_aggregate2', None) aggr_no_az = self.api.add_host_to_aggregate(self.context, - aggr_no_az['id'], + aggr_no_az.id, fake_host) - self.assertIn(fake_host, aggr['hosts']) - self.assertIn(fake_host, aggr_no_az['hosts']) + self.assertIn(fake_host, aggr.hosts) + self.assertIn(fake_host, aggr_no_az.hosts) def test_add_host_to_multi_az(self): # Ensure we can't add a host to different availability zone @@ -10595,14 +10595,14 @@ class ComputeAPIAggrTestCase(BaseTestCase): aggr = self.api.create_aggregate(self.context, 'fake_aggregate', fake_zone) aggr = self.api.add_host_to_aggregate(self.context, - aggr['id'], fake_host) - self.assertEqual(len(aggr['hosts']), 1) + aggr.id, fake_host) + self.assertEqual(len(aggr.hosts), 1) fake_zone2 = "another_zone" aggr2 = self.api.create_aggregate(self.context, 'fake_aggregate2', fake_zone2) self.assertRaises(exception.InvalidAggregateActionAdd, self.api.add_host_to_aggregate, - self.context, aggr2['id'], fake_host) + self.context, aggr2.id, fake_host) def test_add_host_to_multi_az_with_nova_agg(self): # Ensure we can't add a host if already existing in an agg with AZ set @@ -10613,14 +10613,14 @@ class ComputeAPIAggrTestCase(BaseTestCase): 'fake_aggregate', CONF.default_availability_zone) aggr = self.api.add_host_to_aggregate(self.context, - aggr['id'], fake_host) - self.assertEqual(len(aggr['hosts']), 1) + aggr.id, fake_host) + self.assertEqual(len(aggr.hosts), 1) fake_zone2 = "another_zone" aggr2 = self.api.create_aggregate(self.context, 'fake_aggregate2', fake_zone2) self.assertRaises(exception.InvalidAggregateActionAdd, self.api.add_host_to_aggregate, - self.context, aggr2['id'], fake_host) + self.context, aggr2.id, fake_host) def test_add_host_to_aggregate_multiple(self): # Ensure we can add multiple hosts to an aggregate. @@ -10630,8 +10630,8 @@ class ComputeAPIAggrTestCase(BaseTestCase): 'fake_aggregate', fake_zone) for host in values[0][1]: aggr = self.api.add_host_to_aggregate(self.context, - aggr['id'], host) - self.assertEqual(len(aggr['hosts']), len(values[0][1])) + aggr.id, host) + self.assertEqual(len(aggr.hosts), len(values[0][1])) def test_add_host_to_aggregate_raise_not_found(self): # Ensure ComputeHostNotFound is raised when adding invalid host. @@ -10640,7 +10640,7 @@ class ComputeAPIAggrTestCase(BaseTestCase): fake_notifier.NOTIFICATIONS = [] self.assertRaises(exception.ComputeHostNotFound, self.api.add_host_to_aggregate, - self.context, aggr['id'], 'invalid_host') + self.context, aggr.id, 'invalid_host') self.assertEqual(len(fake_notifier.NOTIFICATIONS), 2) self.assertEqual(fake_notifier.NOTIFICATIONS[1].publisher_id, 'compute.fake-mini') @@ -10653,11 +10653,11 @@ class ComputeAPIAggrTestCase(BaseTestCase): 'fake_aggregate', fake_zone) for host in values[0][1]: aggr = self.api.add_host_to_aggregate(self.context, - aggr['id'], host) + aggr.id, host) host_to_remove = values[0][1][0] def fake_remove_aggregate_host(*args, **kwargs): - hosts = kwargs["aggregate"]["hosts"] + hosts = kwargs["aggregate"].hosts self.assertNotIn(host_to_remove, hosts) self.stubs.Set(self.api.compute_rpcapi, 'remove_aggregate_host', @@ -10671,7 +10671,7 @@ class ComputeAPIAggrTestCase(BaseTestCase): fake_notifier.NOTIFICATIONS = [] expected = self.api.remove_host_from_aggregate(self.context, - aggr['id'], + aggr.id, host_to_remove) self.assertEqual(len(fake_notifier.NOTIFICATIONS), 2) msg = fake_notifier.NOTIFICATIONS[0] @@ -10680,7 +10680,7 @@ class ComputeAPIAggrTestCase(BaseTestCase): msg = fake_notifier.NOTIFICATIONS[1] self.assertEqual(msg.event_type, 'aggregate.removehost.end') - self.assertEqual(len(aggr['hosts']) - 1, len(expected['hosts'])) + self.assertEqual(len(aggr.hosts) - 1, len(expected.hosts)) def test_remove_host_from_aggregate_raise_not_found(self): # Ensure ComputeHostNotFound is raised when removing invalid host. @@ -10689,7 +10689,7 @@ class ComputeAPIAggrTestCase(BaseTestCase): 'fake_zone') self.assertRaises(exception.ComputeHostNotFound, self.api.remove_host_from_aggregate, - self.context, aggr['id'], 'invalid_host') + self.context, aggr.id, 'invalid_host') def test_aggregate_list(self): aggregate = self.api.create_aggregate(self.context, @@ -10700,28 +10700,26 @@ class ComputeAPIAggrTestCase(BaseTestCase): meta_aggregate = self.api.create_aggregate(self.context, 'fake_aggregate2', 'fake_zone2') - self.api.update_aggregate_metadata(self.context, meta_aggregate['id'], + self.api.update_aggregate_metadata(self.context, meta_aggregate.id, metadata) aggregate_list = self.api.get_aggregate_list(self.context) - self.assertIn(aggregate['id'], - map(lambda x: x['id'], aggregate_list)) - self.assertIn(meta_aggregate['id'], - map(lambda x: x['id'], aggregate_list)) + self.assertIn(aggregate.id, + map(lambda x: x.id, aggregate_list)) + self.assertIn(meta_aggregate.id, + map(lambda x: x.id, aggregate_list)) self.assertIn('fake_aggregate', - map(lambda x: x['name'], aggregate_list)) + map(lambda x: x.name, aggregate_list)) self.assertIn('fake_aggregate2', - map(lambda x: x['name'], aggregate_list)) + map(lambda x: x.name, aggregate_list)) self.assertIn('fake_zone', - map(lambda x: x['availability_zone'], aggregate_list)) + map(lambda x: x.availability_zone, aggregate_list)) self.assertIn('fake_zone2', - map(lambda x: x['availability_zone'], aggregate_list)) - test_meta_aggregate = aggregate_list[1] - self.assertIn('foo_key1', test_meta_aggregate.get('metadata')) - self.assertIn('foo_key2', test_meta_aggregate.get('metadata')) - self.assertEqual('foo_value1', - test_meta_aggregate.get('metadata')['foo_key1']) - self.assertEqual('foo_value2', - test_meta_aggregate.get('metadata')['foo_key2']) + map(lambda x: x.availability_zone, aggregate_list)) + test_agg_meta = getattr(aggregate_list[1], 'metadata', None) + self.assertIn('foo_key1', test_agg_meta) + self.assertIn('foo_key2', test_agg_meta) + self.assertEqual('foo_value1', test_agg_meta['foo_key1']) + self.assertEqual('foo_value2', test_agg_meta['foo_key2']) def test_aggregate_list_with_hosts(self): values = _create_service_entries(self.context) @@ -10729,11 +10727,11 @@ class ComputeAPIAggrTestCase(BaseTestCase): host_aggregate = self.api.create_aggregate(self.context, 'fake_aggregate', fake_zone) - self.api.add_host_to_aggregate(self.context, host_aggregate['id'], + self.api.add_host_to_aggregate(self.context, host_aggregate.id, values[0][1][0]) aggregate_list = self.api.get_aggregate_list(self.context) aggregate = aggregate_list[0] - self.assertIn(values[0][1][0], aggregate.get('hosts')) + self.assertIn(values[0][1][0], getattr(aggregate, 'hosts', None)) class ComputeAPIAggrCallsSchedulerTestCase(test.NoDBTestCase): diff --git a/nova/tests/unit/virt/xenapi/test_xenapi.py b/nova/tests/unit/virt/xenapi/test_xenapi.py index de4cce9c12c9..a74698acc7b9 100644 --- a/nova/tests/unit/virt/xenapi/test_xenapi.py +++ b/nova/tests/unit/virt/xenapi/test_xenapi.py @@ -3059,7 +3059,7 @@ class XenAPIAggregateTestCase(stubs.XenAPITestBase): aggregate = self._aggregate_setup() self.conn._pool.add_to_aggregate(self.context, aggregate, "host") - result = db.aggregate_get(self.context, aggregate['id']) + result = db.aggregate_get(self.context, aggregate.id) self.assertTrue(fake_init_pool.called) self.assertThat(self.fake_metadata, matchers.DictMatches(result['metadetails'])) @@ -3138,7 +3138,7 @@ class XenAPIAggregateTestCase(stubs.XenAPITestBase): aggregate = self._aggregate_setup(metadata=self.fake_metadata) self.conn._pool.remove_from_aggregate(self.context, aggregate, "host") - result = db.aggregate_get(self.context, aggregate['id']) + result = db.aggregate_get(self.context, aggregate.id) self.assertTrue(fake_clear_pool.called) self.assertThat({'availability_zone': 'fake_zone', pool_states.POOL_FLAG: 'XenAPI', @@ -3210,16 +3210,16 @@ class XenAPIAggregateTestCase(stubs.XenAPITestBase): # let's mock the fact that the aggregate is ready! metadata = {pool_states.POOL_FLAG: "XenAPI", pool_states.KEY: pool_states.ACTIVE} - db.aggregate_metadata_add(self.context, aggr['id'], metadata) + db.aggregate_metadata_add(self.context, aggr.id, metadata) for aggregate_host in values[fake_zone]: aggr = self.api.add_host_to_aggregate(self.context, - aggr['id'], aggregate_host) + aggr.id, aggregate_host) # let's mock the fact that the aggregate is in error! expected = self.api.remove_host_from_aggregate(self.context, - aggr['id'], + aggr.id, values[fake_zone][0]) - self.assertEqual(len(aggr['hosts']) - 1, len(expected['hosts'])) - self.assertEqual(expected['metadata'][pool_states.KEY], + self.assertEqual(len(aggr.hosts) - 1, len(expected.hosts)) + self.assertEqual(expected.metadata[pool_states.KEY], pool_states.ACTIVE) def test_remove_host_from_aggregate_invalid_dismissed_status(self): @@ -3309,10 +3309,11 @@ class HypervisorPoolTestCase(test.NoDBTestCase): 'hosts': [], 'metadata': { 'master_compute': 'master', - pool_states.POOL_FLAG: {}, - pool_states.KEY: {} + pool_states.POOL_FLAG: '', + pool_states.KEY: '' } } + fake_aggregate = objects.Aggregate(**fake_aggregate) def test_slave_asks_master_to_add_slave_to_pool(self): slave = ResourcePoolWithStubs() diff --git a/nova/virt/xenapi/pool.py b/nova/virt/xenapi/pool.py index a7b5ef509ea0..79a8992c26a9 100644 --- a/nova/virt/xenapi/pool.py +++ b/nova/virt/xenapi/pool.py @@ -64,27 +64,27 @@ class ResourcePool(object): except Exception: LOG.exception(_LE('Aggregate %(aggregate_id)s: unrecoverable ' 'state during operation on %(host)s'), - {'aggregate_id': aggregate['id'], 'host': host}) + {'aggregate_id': aggregate.id, 'host': host}) def add_to_aggregate(self, context, aggregate, host, slave_info=None): """Add a compute host to an aggregate.""" - if not pool_states.is_hv_pool(aggregate['metadata']): + if not pool_states.is_hv_pool(aggregate.metadata): return invalid = {pool_states.CHANGING: _('setup in progress'), pool_states.DISMISSED: _('aggregate deleted'), pool_states.ERROR: _('aggregate in error')} - if (aggregate['metadata'][pool_states.KEY] in invalid.keys()): + if (aggregate.metadata[pool_states.KEY] in invalid.keys()): raise exception.InvalidAggregateActionAdd( - aggregate_id=aggregate['id'], - reason=invalid[aggregate['metadata'][pool_states.KEY]]) + aggregate_id=aggregate.id, + reason=invalid[aggregate.metadata[pool_states.KEY]]) - if (aggregate['metadata'][pool_states.KEY] == pool_states.CREATED): + if (aggregate.metadata[pool_states.KEY] == pool_states.CREATED): aggregate.update_metadata({pool_states.KEY: pool_states.CHANGING}) - if len(aggregate['hosts']) == 1: + if len(aggregate.hosts) == 1: # this is the first host of the pool -> make it master - self._init_pool(aggregate['id'], aggregate['name']) + self._init_pool(aggregate.id, aggregate.name) # save metadata so that we can find the master again metadata = {'master_compute': host, host: self._host_uuid, @@ -93,12 +93,12 @@ class ResourcePool(object): else: # the pool is already up and running, we need to figure out # whether we can serve the request from this host or not. - master_compute = aggregate['metadata']['master_compute'] + master_compute = aggregate.metadata['master_compute'] if master_compute == CONF.host and master_compute != host: # this is the master -> do a pool-join # To this aim, nova compute on the slave has to go down. # NOTE: it is assumed that ONLY nova compute is running now - self._join_slave(aggregate['id'], host, + self._join_slave(aggregate.id, host, slave_info.get('compute_uuid'), slave_info.get('url'), slave_info.get('user'), slave_info.get('passwd')) @@ -115,47 +115,47 @@ class ResourcePool(object): def remove_from_aggregate(self, context, aggregate, host, slave_info=None): """Remove a compute host from an aggregate.""" slave_info = slave_info or dict() - if not pool_states.is_hv_pool(aggregate['metadata']): + if not pool_states.is_hv_pool(aggregate.metadata): return invalid = {pool_states.CREATED: _('no hosts to remove'), pool_states.CHANGING: _('setup in progress'), pool_states.DISMISSED: _('aggregate deleted')} - if aggregate['metadata'][pool_states.KEY] in invalid.keys(): + if aggregate.metadata[pool_states.KEY] in invalid.keys(): raise exception.InvalidAggregateActionDelete( - aggregate_id=aggregate['id'], - reason=invalid[aggregate['metadata'][pool_states.KEY]]) + aggregate_id=aggregate.id, + reason=invalid[aggregate.metadata[pool_states.KEY]]) - master_compute = aggregate['metadata']['master_compute'] + master_compute = aggregate.metadata['master_compute'] if master_compute == CONF.host and master_compute != host: # this is the master -> instruct it to eject a host from the pool - host_uuid = aggregate['metadata'][host] - self._eject_slave(aggregate['id'], + host_uuid = aggregate.metadata[host] + self._eject_slave(aggregate.id, slave_info.get('compute_uuid'), host_uuid) aggregate.update_metadata({host: None}) elif master_compute == host: # Remove master from its own pool -> destroy pool only if the # master is on its own, otherwise raise fault. Destroying a # pool made only by master is fictional - if len(aggregate['hosts']) > 1: + if len(aggregate.hosts) > 1: # NOTE: this could be avoided by doing a master # re-election, but this is simpler for now. raise exception.InvalidAggregateActionDelete( - aggregate_id=aggregate['id'], + aggregate_id=aggregate.id, reason=_('Unable to eject %s ' 'from the pool; pool not empty') % host) - self._clear_pool(aggregate['id']) + self._clear_pool(aggregate.id) aggregate.update_metadata({'master_compute': None, host: None}) elif master_compute and master_compute != host: # A master exists -> forward pool-eject request to master slave_info = self._create_slave_info() self.compute_rpcapi.remove_aggregate_host( - context, aggregate['id'], host, master_compute, slave_info) + context, aggregate.id, host, master_compute, slave_info) else: # this shouldn't have happened - raise exception.AggregateError(aggregate_id=aggregate['id'], + raise exception.AggregateError(aggregate_id=aggregate.id, action='remove_from_aggregate', reason=_('Unable to eject %s ' 'from the pool; No master found')