diff --git a/nova/compute/api.py b/nova/compute/api.py index 2e0c73dd0f45..141608a310b2 100644 --- a/nova/compute/api.py +++ b/nova/compute/api.py @@ -5267,7 +5267,8 @@ class AggregateAPI(base.Base): aggregate.name = values.pop('name') aggregate.save() self.is_safe_to_update_az(context, values, aggregate=aggregate, - action_name=AGGREGATE_ACTION_UPDATE) + action_name=AGGREGATE_ACTION_UPDATE, + check_no_instances_in_az=True) if values: aggregate.update_metadata(values) aggregate.updated_at = timeutils.utcnow() @@ -5283,7 +5284,8 @@ class AggregateAPI(base.Base): """Updates the aggregate metadata.""" aggregate = objects.Aggregate.get_by_id(context, aggregate_id) self.is_safe_to_update_az(context, metadata, aggregate=aggregate, - action_name=AGGREGATE_ACTION_UPDATE_META) + action_name=AGGREGATE_ACTION_UPDATE_META, + check_no_instances_in_az=True) aggregate.update_metadata(metadata) self.query_client.update_aggregates(context, [aggregate]) # If updated metadata include availability_zones, then the cache @@ -5325,7 +5327,8 @@ class AggregateAPI(base.Base): def is_safe_to_update_az(self, context, metadata, aggregate, hosts=None, - action_name=AGGREGATE_ACTION_ADD): + action_name=AGGREGATE_ACTION_ADD, + check_no_instances_in_az=False): """Determine if updates alter an aggregate's availability zone. :param context: local context @@ -5333,7 +5336,9 @@ class AggregateAPI(base.Base): :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 + :param action_name: Calling method for logging purposes + :param check_no_instances_in_az: if True, it checks + there is no instances on any hosts of the aggregate """ if 'availability_zone' in metadata: @@ -5354,6 +5359,18 @@ class AggregateAPI(base.Base): "%s") % conflicting_azs self._raise_invalid_aggregate_exc(action_name, aggregate.id, msg) + same_az_name = (aggregate.availability_zone == + metadata['availability_zone']) + if check_no_instances_in_az and not same_az_name: + instance_count_by_cell = ( + nova_context.scatter_gather_skip_cell0( + context, + objects.InstanceList.get_count_by_hosts, + _hosts)) + if any(cnt for cnt in instance_count_by_cell.values()): + msg = _("One or more hosts contain instances in this zone") + self._raise_invalid_aggregate_exc( + action_name, aggregate.id, msg) def _raise_invalid_aggregate_exc(self, action_name, aggregate_id, reason): if action_name == AGGREGATE_ACTION_ADD: diff --git a/nova/objects/instance.py b/nova/objects/instance.py index 15c07663cc10..55ae2170be6e 100644 --- a/nova/objects/instance.py +++ b/nova/objects/instance.py @@ -1570,3 +1570,14 @@ class InstanceList(base.ObjectListBase, base.NovaObject): 'ram': }} """ return cls._get_counts_in_db(context, project_id, user_id=user_id) + + @staticmethod + @db_api.pick_context_manager_reader + def _get_count_by_hosts(context, hosts): + return context.session.query(models.Instance).\ + filter_by(deleted=0).\ + filter(models.Instance.host.in_(hosts)).count() + + @classmethod + def get_count_by_hosts(cls, context, hosts): + return cls._get_count_by_hosts(context, hosts) diff --git a/nova/tests/functional/db/test_instance.py b/nova/tests/functional/db/test_instance.py index e39ef6abe780..e2270c227489 100644 --- a/nova/tests/functional/db/test_instance.py +++ b/nova/tests/functional/db/test_instance.py @@ -122,3 +122,17 @@ class InstanceObjectTestCase(test.TestCase): self.assertEqual(1, count_hit) inst3 = objects.Instance.get_by_uuid(self.context, uuid3) self.assertEqual('nova-test', inst3.availability_zone) + + def test_get_count_by_hosts(self): + self._create_instance(host='fake_host1') + self._create_instance(host='fake_host1') + self._create_instance(host='fake_host2') + count = objects.InstanceList.get_count_by_hosts( + self.context, hosts=['fake_host1']) + self.assertEqual(2, count) + count = objects.InstanceList.get_count_by_hosts( + self.context, hosts=['fake_host2']) + self.assertEqual(1, count) + count = objects.InstanceList.get_count_by_hosts( + self.context, hosts=['fake_host1', 'fake_host2']) + self.assertEqual(3, count) diff --git a/nova/tests/functional/test_aggregates.py b/nova/tests/functional/test_aggregates.py index ead5eacc58bb..0a23839ba96d 100644 --- a/nova/tests/functional/test_aggregates.py +++ b/nova/tests/functional/test_aggregates.py @@ -21,6 +21,7 @@ from nova import context as nova_context from nova.scheduler import weights from nova import test from nova.tests import fixtures as nova_fixtures +from nova.tests.functional.api import client from nova.tests.functional import fixtures as func_fixtures from nova.tests.functional import integrated_helpers import nova.tests.unit.image.fake @@ -191,6 +192,22 @@ class AggregateRequestFiltersTest(test.TestCase, } self.admin_api.post_aggregate_action(agg['id'], action) + def _set_metadata(self, agg, metadata): + """POST /os-aggregates/{aggregate_id}/action (set_metadata) + + :param agg: Name of the nova aggregate + :param metadata: dict of aggregate metadata key/value pairs to add, + update, or remove if value=None (note "availability_zone" cannot be + nulled out once set). + """ + agg = self.aggregates[agg] + action = { + 'set_metadata': { + 'metadata': metadata + }, + } + self.admin_api.post_aggregate_action(agg['id'], action) + def _grant_tenant_aggregate(self, agg, tenants): """Grant a set of tenants access to use an aggregate. @@ -209,6 +226,87 @@ class AggregateRequestFiltersTest(test.TestCase, self.admin_api.post_aggregate_action(agg['id'], action) +class AggregatePostTest(AggregateRequestFiltersTest): + + def test_set_az_for_aggreate_no_instances(self): + """Should be possible to update AZ for an empty aggregate. + + Check you can change the AZ name of an aggregate when it does + not contain any servers. + """ + self._set_az_aggregate('only-host1', 'fake-az') + + def test_rename_to_same_az(self): + """AZ rename should pass successfully if AZ name is not changed""" + az = 'fake-az' + self._set_az_aggregate('only-host1', az) + self._boot_server(az=az) + self._set_az_aggregate('only-host1', az) + + def test_fail_set_az(self): + """Check it is not possible to update a non-empty aggregate. + + Check you cannot change the AZ name of an aggregate when it + contains any servers. + """ + az = 'fake-az' + self._set_az_aggregate('only-host1', az) + server = self._boot_server(az=az) + self.assertRaisesRegex( + client.OpenStackApiException, + 'One or more hosts contain instances in this zone.', + self._set_az_aggregate, 'only-host1', 'new' + az) + + # Configure for the SOFT_DELETED scenario. + self.flags(reclaim_instance_interval=300) + self.api.delete_server(server['id']) + server = self._wait_for_state_change(server, from_status='ACTIVE') + self.assertEqual('SOFT_DELETED', server['status']) + self.assertRaisesRegex( + client.OpenStackApiException, + 'One or more hosts contain instances in this zone.', + self._set_az_aggregate, 'only-host1', 'new' + az) + # Force delete the SOFT_DELETED server. + self.api.api_post( + '/servers/%s/action' % server['id'], {'forceDelete': None}) + # Wait for it to be deleted since forceDelete is asynchronous. + self._wait_until_deleted(server) + # Now we can rename the AZ since the server is gone. + self._set_az_aggregate('only-host1', 'new' + az) + + def test_cannot_delete_az(self): + az = 'fake-az' + # Assign the AZ to the aggregate. + self._set_az_aggregate('only-host1', az) + # Set some metadata on the aggregate; note the "availability_zone" + # metadata key is not specified. + self._set_metadata('only-host1', {'foo': 'bar'}) + # Verify the AZ was retained. + agg = self.admin_api.api_get( + '/os-aggregates/%s' % + self.aggregates['only-host1']['id']).body['aggregate'] + self.assertEqual(az, agg['availability_zone']) + + +# NOTE: this test case has the same test methods as AggregatePostTest +# but for the AZ update it uses PUT /os-aggregates/{aggregate_id} method +class AggregatePutTest(AggregatePostTest): + + def _set_az_aggregate(self, agg, az): + """Set the availability_zone of an aggregate via PUT + + :param agg: Name of the nova aggregate + :param az: Availability zone name + """ + agg = self.aggregates[agg] + body = { + 'aggregate': { + 'availability_zone': az, + }, + } + self.admin_api.put_aggregate(agg['id'], body) + + class TenantAggregateFilterTest(AggregateRequestFiltersTest): def setUp(self): super(TenantAggregateFilterTest, self).setUp() diff --git a/releasenotes/notes/bug-1378904-disable-az-rename-b22a558a20b12706.yaml b/releasenotes/notes/bug-1378904-disable-az-rename-b22a558a20b12706.yaml new file mode 100644 index 000000000000..788408ecc8aa --- /dev/null +++ b/releasenotes/notes/bug-1378904-disable-az-rename-b22a558a20b12706.yaml @@ -0,0 +1,7 @@ +--- +fixes: + - | + ``PUT /os-aggregates/{aggregate_id}`` and + ``POST /os-aggregates/{aggregate_id}/action`` (for set_metadata action) will + now return HTTP 400 for availability zone renaming if the hosts of + the aggregate have any instances.