From 0ad5a64dc9ac4b1cfb8038f9171b6385fdf07f28 Mon Sep 17 00:00:00 2001 From: Sylvain Bauza Date: Tue, 8 Apr 2014 11:00:40 +0200 Subject: [PATCH] Fix AvailabilityZone check for hosts in multiple aggregates Check for multiple AZ was distinct when adding an host to an aggregate and when updating an aggregate (or its metadata), which was leading to possible corner-cases where logics were different. This patch proposes a single way for checking for both API methods and also adds an extra control on the possibility to have an aggregate with its metadata AZ set to the configuration default value (eg. 'nova') and where it was possible to subsequently have one host in two disctinct AZs Fixed that scenerio as it is a gap, Nova as of now preventing to have one host in two distinct AZs. Change-Id: Ida6aa507f34f36a436adaafe1f7609963c59a9bc Partial-Bug: #1277230 --- nova/compute/api.py | 85 +++++++++++++++++++----------- nova/tests/compute/test_compute.py | 36 +++++++++++++ 2 files changed, 89 insertions(+), 32 deletions(-) diff --git a/nova/compute/api.py b/nova/compute/api.py index 148343cee85a..28b002177847 100644 --- a/nova/compute/api.py +++ b/nova/compute/api.py @@ -3300,32 +3300,14 @@ class AggregateAPI(base.Base): aggregates = aggregate_obj.AggregateList.get_all(context) return [self._reformat_aggregate_info(agg) for agg in aggregates] - def is_safe_to_update_az(self, context, aggregate, metadata, - action_name): - """Determine if updates alter an aggregate's availability zone.""" - if 'availability_zone' in metadata: - aggregate_az = aggregate.metadata.get("availability_zone") - for host in aggregate.hosts: - host_az = availability_zones.get_host_availability_zone( - context, host) - if (host_az and host_az != metadata["availability_zone"] - and host_az != CONF.default_availability_zone and - host_az != aggregate_az): - msg = _("This aggregate contains hosts in" - " an existing availability zone") - raise exception.InvalidAggregateAction( - action=action_name, - aggregate_id=aggregate.id, - reason=msg) - @wrap_exception() def update_aggregate(self, context, aggregate_id, values): """Update the properties of an aggregate.""" aggregate = aggregate_obj.Aggregate.get_by_id(context, aggregate_id) if 'name' in values: aggregate.name = values.pop('name') - self.is_safe_to_update_az(context, aggregate, - values, "update aggregate") + self.is_safe_to_update_az(context, values, aggregate=aggregate, + action_name="update_aggregate") if values: aggregate.metadata = values aggregate.save() @@ -3339,8 +3321,8 @@ class AggregateAPI(base.Base): def update_aggregate_metadata(self, context, aggregate_id, metadata): """Updates the aggregate metadata.""" aggregate = aggregate_obj.Aggregate.get_by_id(context, aggregate_id) - self.is_safe_to_update_az(context, aggregate, - metadata, "update aggregate metadata") + self.is_safe_to_update_az(context, metadata, aggregate=aggregate, + action_name="update_aggregate_metadata") aggregate.update_metadata(metadata) # If updated metadata include availability_zones, then the cache # which stored availability_zones and host need to be reset @@ -3366,19 +3348,59 @@ class AggregateAPI(base.Base): "delete.end", aggregate_payload) - def _check_az_for_host(self, aggregate_meta, host_az, aggregate_id): + def is_safe_to_update_az(self, context, metadata, aggregate, + hosts=None, action_name="add_host_to_aggregate"): + """Determine if updates alter an aggregate's availability zone. + + :param context: local context + :param metadata: Target metadata for updating aggregate + :param aggregate: Aggregate to update + :param hosts: Hosts to check. If None, aggregate.hosts is used + :type hosts: list + :action_name: Calling method for logging purposes + + """ + if 'availability_zone' in metadata: + _hosts = hosts or aggregate.hosts + for host in _hosts: + host_az = availability_zones.get_host_availability_zone( + context, host) + if host_az == CONF.default_availability_zone: + # NOTE(sbauza): Aggregate with AZ set to default AZ can + # exist, we need to check + host_aggs = aggregate_obj.AggregateList.get_by_host( + context, host, key='availability_zone') + default_aggs = [agg for agg in host_aggs + if agg['metadata'].get( + 'availability_zone' + ) == CONF.default_availability_zone] + else: + default_aggs = [] + if (host_az != aggregate.metadata.get('availability_zone') and + (host_az != CONF.default_availability_zone or + len(default_aggs) != 0)): + self._check_az_for_host( + metadata, host_az, aggregate.id, + action_name=action_name) + + def _check_az_for_host(self, aggregate_meta, host_az, aggregate_id, + action_name="add_host_to_aggregate"): # NOTE(mtreinish) The availability_zone key returns a set of # zones so loop over each zone. However there should only # ever be one zone in the set because an aggregate can only # have a single availability zone set at one time. - for aggregate_az in aggregate_meta["availability_zone"]: + if isinstance(aggregate_meta["availability_zone"], six.string_types): + azs = set([aggregate_meta["availability_zone"]]) + else: + azs = aggregate_meta["availability_zone"] + + for aggregate_az in azs: # NOTE(mtreinish) Ensure that the aggregate_az is not none # if it is none then that is just a regular aggregate and # it is valid to have a host in multiple aggregates. if aggregate_az and aggregate_az != host_az: msg = _("Host already in availability zone " "%s") % host_az - action_name = "add_host_to_aggregate" raise exception.InvalidAggregateAction( action=action_name, aggregate_id=aggregate_id, reason=msg) @@ -3401,14 +3423,13 @@ class AggregateAPI(base.Base): aggregate_payload) # validates the host; ComputeHostNotFound is raised if invalid service_obj.Service.get_by_compute_host(context, host_name) - host_az = availability_zones.get_host_availability_zone(context, - host_name) - if host_az and host_az != CONF.default_availability_zone: - aggregate_meta = self.db.aggregate_metadata_get_by_metadata_key( - context, aggregate_id, 'availability_zone') - if aggregate_meta.get("availability_zone"): - self._check_az_for_host(aggregate_meta, host_az, aggregate_id) + + metadata = self.db.aggregate_metadata_get_by_metadata_key( + context, aggregate_id, 'availability_zone') aggregate = aggregate_obj.Aggregate.get_by_id(context, aggregate_id) + self.is_safe_to_update_az(context, metadata, hosts=[host_name], + aggregate=aggregate) + aggregate.add_host(context, host_name) self._update_az_cache_for_host(context, host_name, aggregate.metadata) #NOTE(jogo): Send message to host to support resource pools diff --git a/nova/tests/compute/test_compute.py b/nova/tests/compute/test_compute.py index 958aec24bc53..f668567ff807 100644 --- a/nova/tests/compute/test_compute.py +++ b/nova/tests/compute/test_compute.py @@ -9321,6 +9321,23 @@ class ComputeAPIAggrTestCase(BaseTestCase): self.assertEqual(msg.event_type, 'aggregate.updateprop.end') + def test_update_aggregate_az_fails_with_nova_az(self): + # Ensure aggregate's availability zone can't be updated, + # when aggregate has hosts in other availability zone + fake_notifier.NOTIFICATIONS = [] + values = _create_service_entries(self.context) + fake_zone = values.keys()[0] + fake_host = values[fake_zone][0] + aggr1 = self._init_aggregate_with_host(None, 'fake_aggregate1', + CONF.default_availability_zone, + fake_host) + aggr2 = self._init_aggregate_with_host(None, 'fake_aggregate2', None, + fake_host) + metadata = {'availability_zone': 'another_zone'} + self.assertRaises(exception.InvalidAggregateAction, + self.api.update_aggregate, + self.context, aggr2['id'], metadata) + def test_update_aggregate_metadata(self): # Ensure metadata can be updated. aggr = self.api.create_aggregate(self.context, 'fake_aggregate', @@ -9547,6 +9564,25 @@ class ComputeAPIAggrTestCase(BaseTestCase): self.api.add_host_to_aggregate, 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 + # to default + values = _create_service_entries(self.context) + fake_zone = values.keys()[0] + fake_host = values[fake_zone][0] + aggr = self.api.create_aggregate(self.context, + '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) + fake_zone2 = "another_zone" + aggr2 = self.api.create_aggregate(self.context, + 'fake_aggregate2', fake_zone2) + self.assertRaises(exception.InvalidAggregateAction, + self.api.add_host_to_aggregate, + self.context, aggr2['id'], fake_host) + def test_add_host_to_aggregate_multiple(self): # Ensure we can add multiple hosts to an aggregate. values = _create_service_entries(self.context)