diff --git a/nova/api/openstack/compute/migrate_server.py b/nova/api/openstack/compute/migrate_server.py index 5c7ef1def454..b106ad8742ab 100644 --- a/nova/api/openstack/compute/migrate_server.py +++ b/nova/api/openstack/compute/migrate_server.py @@ -56,7 +56,7 @@ class MigrateServerController(wsgi.Controller): host_name = body['migrate'].get('host') instance = common.get_instance(self.compute_api, context, id, - expected_attrs=['flavor']) + expected_attrs=['flavor', 'services']) if common.instance_has_port_with_resource_request( instance.uuid, self.network_api): @@ -76,7 +76,9 @@ class MigrateServerController(wsgi.Controller): host_name=host_name) except (exception.TooManyInstances, exception.QuotaError) as e: raise exc.HTTPForbidden(explanation=e.format_message()) - except exception.InstanceIsLocked as e: + except (exception.InstanceIsLocked, + exception.InstanceNotReady, + exception.ServiceUnavailable) as e: raise exc.HTTPConflict(explanation=e.format_message()) except exception.InstanceInvalidState as state_error: common.raise_http_conflict_for_instance_invalid_state(state_error, diff --git a/nova/api/openstack/compute/servers.py b/nova/api/openstack/compute/servers.py index 5f16a339085c..45b721da251f 100644 --- a/nova/api/openstack/compute/servers.py +++ b/nova/api/openstack/compute/servers.py @@ -348,7 +348,7 @@ class ServersController(wsgi.Controller): return response def _get_server(self, context, req, instance_uuid, is_detail=False, - cell_down_support=False): + cell_down_support=False, columns_to_join=None): """Utility function for looking up an instance by uuid. :param context: request context for auth @@ -360,6 +360,8 @@ class ServersController(wsgi.Controller): returning a minimal instance construct if the relevant cell is down. + :param columns_to_join: optional list of extra fields to join on the + Instance object """ expected_attrs = ['flavor', 'numa_topology'] if is_detail: @@ -369,6 +371,8 @@ class ServersController(wsgi.Controller): expected_attrs.append("trusted_certs") expected_attrs = self._view_builder.get_show_expected_attrs( expected_attrs) + if columns_to_join: + expected_attrs.extend(columns_to_join) instance = common.get_instance(self.compute_api, context, instance_uuid, expected_attrs=expected_attrs, @@ -931,7 +935,8 @@ class ServersController(wsgi.Controller): def _resize(self, req, instance_id, flavor_id, auto_disk_config=None): """Begin the resize process with given instance/flavor.""" context = req.environ["nova.context"] - instance = self._get_server(context, req, instance_id) + instance = self._get_server(context, req, instance_id, + columns_to_join=['services']) context.can(server_policies.SERVERS % 'resize', target={'user_id': instance.user_id, 'project_id': instance.project_id}) @@ -954,7 +959,9 @@ class ServersController(wsgi.Controller): except exception.QuotaError as error: raise exc.HTTPForbidden( explanation=error.format_message()) - except exception.InstanceIsLocked as e: + except (exception.InstanceIsLocked, + exception.InstanceNotReady, + exception.ServiceUnavailable) as e: raise exc.HTTPConflict(explanation=e.format_message()) except exception.InstanceInvalidState as state_error: common.raise_http_conflict_for_instance_invalid_state(state_error, diff --git a/nova/compute/api.py b/nova/compute/api.py index 89020feb526a..f2a4987e992b 100644 --- a/nova/compute/api.py +++ b/nova/compute/api.py @@ -186,13 +186,34 @@ def reject_instance_state(vm_state=None, task_state=None): return outer -def check_instance_host(function): - @six.wraps(function) - def wrapped(self, context, instance, *args, **kwargs): - if not instance.host: - raise exception.InstanceNotReady(instance_id=instance.uuid) - return function(self, context, instance, *args, **kwargs) - return wrapped +def check_instance_host(check_is_up=False): + """Validate the instance.host before performing the operation. + + At a minimum this method will check that the instance.host is set. + + :param check_is_up: If True, check that the instance.host status is UP + or MAINTENANCE (disabled but not down). + :raises: InstanceNotReady if the instance.host is not set + :raises: ServiceUnavailable if check_is_up=True and the instance.host + compute service status is not UP or MAINTENANCE + """ + def outer(function): + @six.wraps(function) + def wrapped(self, context, instance, *args, **kwargs): + if not instance.host: + raise exception.InstanceNotReady(instance_id=instance.uuid) + if check_is_up: + # Make sure the source compute service is not down otherwise we + # cannot proceed. + host_status = self.get_instance_host_status(instance) + if host_status not in (fields_obj.HostStatus.UP, + fields_obj.HostStatus.MAINTENANCE): + # ComputeServiceUnavailable would make more sense here but + # we do not want to leak hostnames to end users. + raise exception.ServiceUnavailable() + return function(self, context, instance, *args, **kwargs) + return wrapped + return outer def check_instance_lock(function): @@ -2519,14 +2540,14 @@ class API(base.Base): clean_shutdown=clean_shutdown) @check_instance_lock - @check_instance_host + @check_instance_host() @check_instance_state(vm_state=[vm_states.ACTIVE, vm_states.ERROR]) def stop(self, context, instance, do_cast=True, clean_shutdown=True): """Stop an instance.""" self.force_stop(context, instance, do_cast, clean_shutdown) @check_instance_lock - @check_instance_host + @check_instance_host() @check_instance_state(vm_state=[vm_states.STOPPED]) def start(self, context, instance): """Start an instance.""" @@ -2539,7 +2560,7 @@ class API(base.Base): self.compute_rpcapi.start_instance(context, instance) @check_instance_lock - @check_instance_host + @check_instance_host() @check_instance_state(vm_state=vm_states.ALLOW_TRIGGER_CRASH_DUMP) def trigger_crash_dump(self, context, instance): """Trigger crash dump in an instance.""" @@ -3783,6 +3804,7 @@ class API(base.Base): @check_instance_lock @check_instance_state(vm_state=[vm_states.ACTIVE, vm_states.STOPPED]) + @check_instance_host(check_is_up=True) def resize(self, context, instance, flavor_id=None, clean_shutdown=True, host_name=None, auto_disk_config=None): """Resize (ie, migrate) a running instance. @@ -4074,12 +4096,12 @@ class API(base.Base): self._record_action_start(context, instance, instance_actions.UNPAUSE) self.compute_rpcapi.unpause_instance(context, instance) - @check_instance_host + @check_instance_host() def get_diagnostics(self, context, instance): """Retrieve diagnostics for the given instance.""" return self.compute_rpcapi.get_diagnostics(context, instance=instance) - @check_instance_host + @check_instance_host() def get_instance_diagnostics(self, context, instance): """Retrieve diagnostics for the given instance.""" return self.compute_rpcapi.get_instance_diagnostics(context, @@ -4161,7 +4183,7 @@ class API(base.Base): instance=instance, new_pass=password) - @check_instance_host + @check_instance_host() @reject_instance_state( task_state=[task_states.DELETING, task_states.MIGRATING]) def get_vnc_console(self, context, instance, console_type): @@ -4170,7 +4192,7 @@ class API(base.Base): instance=instance, console_type=console_type) return {'url': connect_info['access_url']} - @check_instance_host + @check_instance_host() @reject_instance_state( task_state=[task_states.DELETING, task_states.MIGRATING]) def get_spice_console(self, context, instance, console_type): @@ -4179,7 +4201,7 @@ class API(base.Base): instance=instance, console_type=console_type) return {'url': connect_info['access_url']} - @check_instance_host + @check_instance_host() @reject_instance_state( task_state=[task_states.DELETING, task_states.MIGRATING]) def get_rdp_console(self, context, instance, console_type): @@ -4188,7 +4210,7 @@ class API(base.Base): instance=instance, console_type=console_type) return {'url': connect_info['access_url']} - @check_instance_host + @check_instance_host() @reject_instance_state( task_state=[task_states.DELETING, task_states.MIGRATING]) def get_serial_console(self, context, instance, console_type): @@ -4197,7 +4219,7 @@ class API(base.Base): instance=instance, console_type=console_type) return {'url': connect_info['access_url']} - @check_instance_host + @check_instance_host() @reject_instance_state( task_state=[task_states.DELETING, task_states.MIGRATING]) def get_mks_console(self, context, instance, console_type): @@ -4206,7 +4228,7 @@ class API(base.Base): instance=instance, console_type=console_type) return {'url': connect_info['access_url']} - @check_instance_host + @check_instance_host() def get_console_output(self, context, instance, tail_length=None): """Get console output for an instance.""" return self.compute_rpcapi.get_console_output(context, @@ -5082,7 +5104,7 @@ class API(base.Base): # We allow creating the snapshot in any vm_state as long as there is # no task being performed on the instance and it has a host. - @check_instance_host + @check_instance_host() @check_instance_state(vm_state=None) def do_volume_snapshot_create(self, context, instance): self.compute_rpcapi.volume_snapshot_create(context, instance, @@ -5104,7 +5126,7 @@ class API(base.Base): # We allow deleting the snapshot in any vm_state as long as there is # no task being performed on the instance and it has a host. - @check_instance_host + @check_instance_host() @check_instance_state(vm_state=None) def do_volume_snapshot_delete(self, context, instance): self.compute_rpcapi.volume_snapshot_delete(context, instance, diff --git a/nova/objects/instance.py b/nova/objects/instance.py index 467a129045c6..2ccbf5287be6 100644 --- a/nova/objects/instance.py +++ b/nova/objects/instance.py @@ -686,6 +686,10 @@ class Instance(base.NovaPersistentObject, base.NovaObject, # NOTE(gibi): tags are not saved through the instance pass + def _save_services(self, context): + # NOTE(mriedem): services are not saved through the instance + pass + @staticmethod def _nullify_flavor_description(flavor_info): """Helper method to nullify descriptions from a set of primitive diff --git a/nova/tests/functional/notification_sample_tests/test_compute_task.py b/nova/tests/functional/notification_sample_tests/test_compute_task.py index aa1a182a8008..c5852dc9bff9 100644 --- a/nova/tests/functional/notification_sample_tests/test_compute_task.py +++ b/nova/tests/functional/notification_sample_tests/test_compute_task.py @@ -99,9 +99,9 @@ class TestComputeTaskNotificationSample( 'hw:numa_cpus.0': '0', 'hw:numa_mem.0': 512}) self._wait_for_notification('instance.create.end') - # Force down the compute node + # Disable the compute node service_id = self.api.get_service_id('nova-compute') - self.admin_api.put_service_force_down(service_id, True) + self.admin_api.put_service(service_id, {'status': 'disabled'}) fake_notifier.reset() diff --git a/nova/tests/functional/wsgi/test_servers.py b/nova/tests/functional/wsgi/test_servers.py index c56003a2e3e7..648338559fa8 100644 --- a/nova/tests/functional/wsgi/test_servers.py +++ b/nova/tests/functional/wsgi/test_servers.py @@ -387,3 +387,65 @@ class EnforceVolumeBackedForZeroDiskFlavorTestCase( server = self.admin_api.post_server({'server': server_req}) server = self._wait_for_state_change(server, 'ERROR') self.assertIn('No valid host', server['fault']['message']) + + +class ResizeCheckInstanceHostTestCase( + integrated_helpers.ProviderUsageBaseTestCase): + """Tests for the check_instance_host decorator used during resize/migrate. + """ + compute_driver = 'fake.MediumFakeDriver' + + def test_resize_source_compute_validation(self, resize=True): + flavors = self.api.get_flavors() + # Start up a compute on which to create a server. + self._start_compute('host1') + # Create a server on host1. + server = self._build_minimal_create_server_request( + name='test_resize_source_compute_validation', + image_uuid=fake_image.get_valid_image_id(), + flavor_id=flavors[0]['id'], + networks='none') + server = self.api.post_server({'server': server}) + server = self._wait_for_state_change(server, 'ACTIVE') + # Check if we're cold migrating. + if resize: + req = {'resize': {'flavorRef': flavors[1]['id']}} + else: + req = {'migrate': None} + # Start up a destination compute. + self._start_compute('host2') + # First, force down the source compute service. + source_service = self.api.get_services( + binary='nova-compute', host='host1')[0] + self.api.put_service(source_service['id'], {'forced_down': True}) + # Try the operation and it should fail with a 409 response. + ex = self.assertRaises(api_client.OpenStackApiException, + self.api.post_server_action, server['id'], req) + self.assertEqual(409, ex.response.status_code) + self.assertIn('Service is unavailable at this time', six.text_type(ex)) + # Now bring the source compute service up but disable it. The operation + # should be allowed in this case since the service is up. + self.api.put_service(source_service['id'], + {'forced_down': False, 'status': 'disabled'}) + self.api.post_server_action(server['id'], req) + server = self._wait_for_state_change(server, 'VERIFY_RESIZE') + # Revert the resize to get the server back to host1. + self.api.post_server_action(server['id'], {'revertResize': None}) + server = self._wait_for_state_change(server, 'ACTIVE') + # Now shelve offload the server so it does not have a host. + self.api.post_server_action(server['id'], {'shelve': None}) + self._wait_for_server_parameter(server, + {'status': 'SHELVED_OFFLOADED', + 'OS-EXT-SRV-ATTR:host': None}) + # Now try the operation again and it should fail with a different + # 409 response. + ex = self.assertRaises(api_client.OpenStackApiException, + self.api.post_server_action, server['id'], req) + self.assertEqual(409, ex.response.status_code) + # This error comes from check_instance_state which is processed before + # check_instance_host. + self.assertIn('while it is in vm_state shelved_offloaded', + six.text_type(ex)) + + def test_cold_migrate_source_compute_validation(self): + self.test_resize_source_compute_validation(resize=False) diff --git a/nova/tests/unit/api/openstack/compute/admin_only_action_common.py b/nova/tests/unit/api/openstack/compute/admin_only_action_common.py index 27dc6444b800..d55ddf1681d8 100644 --- a/nova/tests/unit/api/openstack/compute/admin_only_action_common.py +++ b/nova/tests/unit/api/openstack/compute/admin_only_action_common.py @@ -52,7 +52,7 @@ class CommonMixin(object): if action == '_migrate_live': expected_attrs = ['numa_topology'] elif action == '_migrate': - expected_attrs = ['flavor'] + expected_attrs = ['flavor', 'services'] uuid = uuidutils.generate_uuid() self.mock_get.side_effect = exception.InstanceNotFound( @@ -75,7 +75,7 @@ class CommonMixin(object): if action == '_migrate_live': expected_attrs = ['numa_topology'] elif action == '_migrate': - expected_attrs = ['flavor'] + expected_attrs = ['flavor', 'services'] if method is None: method = action.replace('_', '') @@ -139,7 +139,7 @@ class CommonMixin(object): if action == '_migrate_live': expected_attrs = ['numa_topology'] elif action == '_migrate': - expected_attrs = ['flavor'] + expected_attrs = ['flavor', 'services'] if method is None: method = action.replace('_', '') @@ -180,7 +180,7 @@ class CommonMixin(object): if action == '_migrate_live': expected_attrs = ['numa_topology'] elif action == '_migrate': - expected_attrs = ['flavor'] + expected_attrs = ['flavor', 'services'] if method is None: method = action.replace('_', '') diff --git a/nova/tests/unit/api/openstack/compute/test_migrate_server.py b/nova/tests/unit/api/openstack/compute/test_migrate_server.py index fca646c3287d..089b6f089d9a 100644 --- a/nova/tests/unit/api/openstack/compute/test_migrate_server.py +++ b/nova/tests/unit/api/openstack/compute/test_migrate_server.py @@ -131,9 +131,9 @@ class MigrateServerTestsV21(admin_only_action_common.CommonTests): body={'migrate': None}) mock_resize.assert_called_once_with( self.context, instance, host_name=self.host_name) - self.mock_get.assert_called_once_with(self.context, instance.uuid, - expected_attrs=['flavor'], - cell_down_support=False) + self.mock_get.assert_called_once_with( + self.context, instance.uuid, expected_attrs=['flavor', 'services'], + cell_down_support=False) def test_migrate_too_many_instances(self): exc_info = exception.TooManyInstances(overs='', req='', used=0, diff --git a/nova/tests/unit/api/openstack/compute/test_server_actions.py b/nova/tests/unit/api/openstack/compute/test_server_actions.py index 96931f92ef9a..997ec253e1de 100644 --- a/nova/tests/unit/api/openstack/compute/test_server_actions.py +++ b/nova/tests/unit/api/openstack/compute/test_server_actions.py @@ -138,9 +138,11 @@ class ServerActionsControllerTestV21(test.TestCase): eval(controller_function), self.req, instance['uuid'], body=body_map.get(action)) - + expected_attrs = ['flavor', 'numa_topology'] + if method == 'resize': + expected_attrs.append('services') mock_get.assert_called_once_with(self.context, uuid, - expected_attrs=['flavor', 'numa_topology'], + expected_attrs=expected_attrs, cell_down_support=False) mock_method.assert_called_once_with(self.context, instance, *args, **kwargs) diff --git a/nova/tests/unit/compute/test_compute.py b/nova/tests/unit/compute/test_compute.py index fab17e92ded2..e6660a77ff36 100644 --- a/nova/tests/unit/compute/test_compute.py +++ b/nova/tests/unit/compute/test_compute.py @@ -12509,6 +12509,8 @@ class DisabledInstanceTypesTestCase(BaseTestCase): self.assertRaises(exception.FlavorNotFound, self.compute_api.create, self.context, self.inst_type, None) + @mock.patch('nova.compute.api.API.get_instance_host_status', + new=mock.Mock(return_value=obj_fields.HostStatus.UP)) @mock.patch('nova.compute.api.API._validate_flavor_image_nostatus') @mock.patch('nova.objects.RequestSpec') def test_can_resize_to_visible_instance_type(self, mock_reqspec, @@ -12531,6 +12533,8 @@ class DisabledInstanceTypesTestCase(BaseTestCase): with mock.patch('nova.conductor.api.ComputeTaskAPI.resize_instance'): self.compute_api.resize(self.context, instance, '4') + @mock.patch('nova.compute.api.API.get_instance_host_status', + new=mock.Mock(return_value=obj_fields.HostStatus.UP)) def test_cannot_resize_to_disabled_instance_type(self): instance = self._create_fake_instance_obj() orig_get_flavor_by_flavor_id = \ diff --git a/nova/tests/unit/compute/test_compute_api.py b/nova/tests/unit/compute/test_compute_api.py index 9e6b37cd0bd7..c8820db75c40 100644 --- a/nova/tests/unit/compute/test_compute_api.py +++ b/nova/tests/unit/compute/test_compute_api.py @@ -1752,6 +1752,8 @@ class _ComputeAPIUnitTestMixIn(object): mock_inst_save.assert_called_once_with(expected_task_state=[None]) mock_get_requested_resources.assert_not_called() + @mock.patch('nova.compute.api.API.get_instance_host_status', + new=mock.Mock(return_value=fields_obj.HostStatus.UP)) @mock.patch('nova.compute.utils.is_volume_backed_instance', return_value=False) @mock.patch('nova.compute.api.API._validate_flavor_image_nostatus') @@ -1966,6 +1968,8 @@ class _ComputeAPIUnitTestMixIn(object): def test_resize_allow_cross_cell_resize_true(self): self._test_resize(allow_cross_cell_resize=True) + @mock.patch('nova.compute.api.API.get_instance_host_status', + new=mock.Mock(return_value=fields_obj.HostStatus.UP)) @mock.patch('nova.compute.flavors.get_flavor_by_flavor_id') @mock.patch('nova.objects.Quotas.count_as_dict') @mock.patch('nova.objects.Quotas.limit_check_project_and_user') @@ -2023,6 +2027,8 @@ class _ComputeAPIUnitTestMixIn(object): self._test_migrate(host_name='target_host', allow_cross_cell_resize=True) + @mock.patch('nova.compute.api.API.get_instance_host_status', + new=mock.Mock(return_value=fields_obj.HostStatus.UP)) @mock.patch.object(objects.ComputeNodeList, 'get_all_by_host', side_effect=exception.ComputeHostNotFound( host='nonexistent_host')) @@ -2032,6 +2038,8 @@ class _ComputeAPIUnitTestMixIn(object): self.compute_api.resize, self.context, fake_inst, host_name='nonexistent_host') + @mock.patch('nova.compute.api.API.get_instance_host_status', + new=mock.Mock(return_value=fields_obj.HostStatus.UP)) @mock.patch.object(objects.Instance, 'save') @mock.patch.object(compute_api.API, '_record_action_start') @mock.patch.object(quotas_obj.Quotas, 'limit_check_project_and_user') @@ -2057,6 +2065,8 @@ class _ComputeAPIUnitTestMixIn(object): mock_resize.assert_not_called() mock_save.assert_not_called() + @mock.patch('nova.compute.api.API.get_instance_host_status', + new=mock.Mock(return_value=fields_obj.HostStatus.UP)) @mock.patch.object(objects.Instance, 'save') @mock.patch.object(compute_api.API, '_record_action_start') @mock.patch.object(quotas_obj.Quotas, 'limit_check_project_and_user') @@ -2083,6 +2093,8 @@ class _ComputeAPIUnitTestMixIn(object): mock_resize.assert_not_called() mock_save.assert_not_called() + @mock.patch('nova.compute.api.API.get_instance_host_status', + new=mock.Mock(return_value=fields_obj.HostStatus.UP)) @mock.patch.object(flavors, 'get_flavor_by_flavor_id') def test_resize_to_zero_disk_flavor_fails(self, get_flavor_by_flavor_id): fake_inst = self._create_instance_obj() @@ -2097,6 +2109,8 @@ class _ComputeAPIUnitTestMixIn(object): self.compute_api.resize, self.context, fake_inst, flavor_id='flavor-id') + @mock.patch('nova.compute.api.API.get_instance_host_status', + new=mock.Mock(return_value=fields_obj.HostStatus.UP)) @mock.patch('nova.compute.api.API._validate_flavor_image_nostatus') @mock.patch.object(objects.RequestSpec, 'get_by_instance_uuid') @mock.patch('nova.compute.api.API._record_action_start') @@ -2126,6 +2140,8 @@ class _ComputeAPIUnitTestMixIn(object): do_test() + @mock.patch('nova.compute.api.API.get_instance_host_status', + new=mock.Mock(return_value=fields_obj.HostStatus.UP)) @mock.patch.object(objects.Instance, 'save') @mock.patch.object(compute_api.API, '_record_action_start') @mock.patch.object(quotas_obj.Quotas, @@ -2182,6 +2198,8 @@ class _ComputeAPIUnitTestMixIn(object): mock_record.assert_not_called() mock_resize.assert_not_called() + @mock.patch('nova.compute.api.API.get_instance_host_status', + new=mock.Mock(return_value=fields_obj.HostStatus.UP)) @mock.patch.object(flavors, 'get_flavor_by_flavor_id') @mock.patch.object(compute_utils, 'upsize_quota_delta') @mock.patch.object(quotas_obj.Quotas, 'count_as_dict') @@ -2207,6 +2225,8 @@ class _ComputeAPIUnitTestMixIn(object): fake_inst, flavor_id='flavor-id') self.assertFalse(mock_save.called) + @mock.patch('nova.compute.api.API.get_instance_host_status', + new=mock.Mock(return_value=fields_obj.HostStatus.UP)) @mock.patch.object(flavors, 'get_flavor_by_flavor_id') @mock.patch.object(objects.Quotas, 'count_as_dict') @mock.patch.object(objects.Quotas, 'limit_check_project_and_user') @@ -3502,6 +3522,8 @@ class _ComputeAPIUnitTestMixIn(object): lambda obj, context, image_id, **kwargs: self.fake_image) return self.fake_image['id'] + @mock.patch('nova.compute.api.API.get_instance_host_status', + new=mock.Mock(return_value=fields_obj.HostStatus.UP)) def test_resize_with_disabled_auto_disk_config_fails(self): fake_inst = self._create_instance_with_disabled_disk_config( object=True) diff --git a/releasenotes/notes/bug-1856925-check-source-compute-resize-16e9c3b24cf72301.yaml b/releasenotes/notes/bug-1856925-check-source-compute-resize-16e9c3b24cf72301.yaml new file mode 100644 index 000000000000..76b20a39750b --- /dev/null +++ b/releasenotes/notes/bug-1856925-check-source-compute-resize-16e9c3b24cf72301.yaml @@ -0,0 +1,8 @@ +--- +fixes: + - | + This release contains a fix for `bug 1856925`_ such that ``resize`` and + ``migrate`` server actions will be rejected with a 409 ``HTTPConflict`` + response if the source compute service is down. + + .. _bug 1856925: https://bugs.launchpad.net/nova/+bug/1856925