diff --git a/nova/compute/api.py b/nova/compute/api.py index b96fb15f825c..21976186443e 100644 --- a/nova/compute/api.py +++ b/nova/compute/api.py @@ -2949,9 +2949,24 @@ class API(base.Base): # sure that all our instances are currently migrated to have an # attached RequestSpec object but let's consider that the operator only # half migrated all their instances in the meantime. + host = instance.host try: request_spec = objects.RequestSpec.get_by_instance_uuid( context, instance.uuid) + # If a new image is provided on rebuild, we will need to run + # through the scheduler again, but we want the instance to be + # rebuilt on the same host it's already on. + if orig_image_ref != image_href: + request_spec.requested_destination = objects.Destination( + host=instance.host, + node=instance.node) + # We have to modify the request spec that goes to the scheduler + # to contain the new image. We persist this since we've already + # changed the instance.image_ref above so we're being + # consistent. + request_spec.image = objects.ImageMeta.from_dict(image) + request_spec.save() + host = None # This tells conductor to call the scheduler. except exception.RequestSpecNotFound: # Some old instances can still have no RequestSpec object attached # to them, we need to support the old way @@ -2961,7 +2976,7 @@ class API(base.Base): new_pass=admin_password, injected_files=files_to_inject, image_ref=image_href, orig_image_ref=orig_image_ref, orig_sys_metadata=orig_sys_metadata, bdms=bdms, - preserve_ephemeral=preserve_ephemeral, host=instance.host, + preserve_ephemeral=preserve_ephemeral, host=host, request_spec=request_spec, kwargs=kwargs) diff --git a/nova/conductor/manager.py b/nova/conductor/manager.py index f7361345ec0e..4a5fdddfcbde 100644 --- a/nova/conductor/manager.py +++ b/nova/conductor/manager.py @@ -801,7 +801,8 @@ class ComputeTaskManager(base.Base): # The host variable is passed in two cases: # 1. rebuild - the instance.host is passed to rebuild on the - # same host and bypass the scheduler. + # same host and bypass the scheduler *unless* a new image + # was specified # 2. evacuate with specified host and force=True - the specified # host is passed and is meant to bypass the scheduler. # NOTE(mriedem): This could be a lot more straight-forward if we @@ -815,10 +816,13 @@ class ComputeTaskManager(base.Base): self._allocate_for_evacuate_dest_host( context, instance, host, request_spec) else: - # At this point, either the user is doing a rebuild on the - # same host (not evacuate), or they are evacuating and - # specified a host but are not forcing it. The API passes - # host=None in this case but sets up the + # At this point, the user is either: + # + # 1. Doing a rebuild on the same host (not evacuate) and + # specified a new image. + # 2. Evacuating and specified a host but are not forcing it. + # + # In either case, the API passes host=None but sets up the # RequestSpec.requested_destination field for the specified # host. if not request_spec: @@ -833,7 +837,7 @@ class ComputeTaskManager(base.Base): image_meta, [instance]) request_spec = objects.RequestSpec.from_primitives( context, request_spec, filter_properties) - else: + elif recreate: # NOTE(sbauza): Augment the RequestSpec object by excluding # the source host for avoiding the scheduler to pick it request_spec.ignore_hosts = request_spec.ignore_hosts or [] diff --git a/nova/tests/functional/api/client.py b/nova/tests/functional/api/client.py index 3d557221e524..b44423a26bcf 100644 --- a/nova/tests/functional/api/client.py +++ b/nova/tests/functional/api/client.py @@ -306,6 +306,16 @@ class TestOpenStackClient(object): def delete_image(self, image_id): return self.api_delete('/images/%s' % image_id) + def put_image_meta_key(self, image_id, key, value): + """Creates or updates a given image metadata key/value pair.""" + req_body = { + 'meta': { + key: value + } + } + return self.api_put('/images/%s/metadata/%s' % (image_id, key), + req_body) + def get_flavor(self, flavor_id): return self.api_get('/flavors/%s' % flavor_id).body['flavor'] diff --git a/nova/tests/functional/integrated_helpers.py b/nova/tests/functional/integrated_helpers.py index e7754e89610b..50fe6f234581 100644 --- a/nova/tests/functional/integrated_helpers.py +++ b/nova/tests/functional/integrated_helpers.py @@ -270,19 +270,21 @@ class InstanceHelperMixin(object): return def _wait_for_action_fail_completion( - self, server, expected_action, event_name): + self, server, expected_action, event_name, api=None): """Polls instance action events for the given instance, action and action event name until it finds the action event with an error result. """ + if api is None: + api = self.api completion_event = None for attempt in range(10): - actions = self.api.get_instance_actions(server['id']) + actions = api.get_instance_actions(server['id']) # Look for the migrate action. for action in actions: if action['action'] == expected_action: events = ( - self.api.api_get( + api.api_get( '/servers/%s/os-instance-actions/%s' % (server['id'], action['request_id']) ).body['instanceAction']['events']) diff --git a/nova/tests/functional/test_servers.py b/nova/tests/functional/test_servers.py index 690b644c9b2c..7bf94990a7f3 100644 --- a/nova/tests/functional/test_servers.py +++ b/nova/tests/functional/test_servers.py @@ -1059,6 +1059,68 @@ class ServerTestV220(ServersTestBase): self._delete_server(server_id) +class ServerRebuildTestCase(integrated_helpers._IntegratedTestBase, + integrated_helpers.InstanceHelperMixin): + api_major_version = 'v2.1' + # We have to cap the microversion at 2.38 because that's the max we + # can use to update image metadata via our compute images proxy API. + microversion = '2.38' + + def test_rebuild_with_image_novalidhost(self): + """Creates a server with an image that is valid for the single compute + that we have. Then rebuilds the server, passing in an image with + metadata that does not fit the single compute which should result in + a NoValidHost error. The ImagePropertiesFilter filter is enabled by + default so that should filter out the host based on the image meta. + """ + server_req_body = { + 'server': { + # We hard-code from a fake image since we can't get images + # via the compute /images proxy API with microversion > 2.35. + 'imageRef': '155d900f-4e14-4e4c-a73d-069cbf4541e6', + 'flavorRef': '1', # m1.tiny from DefaultFlavorsFixture, + 'name': 'test_rebuild_with_image_novalidhost', + # We don't care about networking for this test. This requires + # microversion >= 2.37. + 'networks': 'none' + } + } + server = self.api.post_server(server_req_body) + self._wait_for_state_change(self.api, server, 'ACTIVE') + # Now update the image metadata to be something that won't work with + # the fake compute driver we're using since the fake driver has an + # "x86_64" architecture. + rebuild_image_ref = ( + nova.tests.unit.image.fake.AUTO_DISK_CONFIG_ENABLED_IMAGE_UUID) + self.api.put_image_meta_key( + rebuild_image_ref, 'hw_architecture', 'unicore32') + # Now rebuild the server with that updated image and it should result + # in a NoValidHost failure from the scheduler. + rebuild_req_body = { + 'rebuild': { + 'imageRef': rebuild_image_ref + } + } + # Since we're using the CastAsCall fixture, the NoValidHost error + # should actually come back to the API and result in a 500 error. + # Normally the user would get a 202 response because nova-api RPC casts + # to nova-conductor which RPC calls the scheduler which raises the + # NoValidHost. We can mimic the end user way to figure out the failure + # by looking for the failed 'rebuild' instance action event. + self.api.api_post('/servers/%s/action' % server['id'], + rebuild_req_body, check_response_status=[500]) + # Look for the failed rebuild action. + self._wait_for_action_fail_completion( + server, instance_actions.REBUILD, 'rebuild_server', + # Before microversion 2.51 events are only returned for instance + # actions if you're an admin. + self.api_fixture.admin_api) + # Unfortunately the server's image_ref is updated to be the new image + # even though the rebuild should not work. + server = self.api.get_server(server['id']) + self.assertEqual(rebuild_image_ref, server['image']['id']) + + class ProviderUsageBaseTestCase(test.TestCase, integrated_helpers.InstanceHelperMixin): """Base test class for functional tests that check provider usage diff --git a/nova/tests/unit/compute/test_compute_api.py b/nova/tests/unit/compute/test_compute_api.py index 660fa64c046c..87c333c6730a 100644 --- a/nova/tests/unit/compute/test_compute_api.py +++ b/nova/tests/unit/compute/test_compute_api.py @@ -3183,6 +3183,7 @@ class _ComputeAPIUnitTestMixIn(object): bdm_get_by_instance_uuid.assert_called_once_with( self.context, instance.uuid) + @mock.patch.object(objects.RequestSpec, 'save') @mock.patch.object(objects.RequestSpec, 'get_by_instance_uuid') @mock.patch.object(objects.Instance, 'save') @mock.patch.object(objects.Instance, 'get_flavor') @@ -3194,7 +3195,7 @@ class _ComputeAPIUnitTestMixIn(object): def test_rebuild_change_image(self, _record_action_start, _checks_for_create_and_rebuild, _check_auto_disk_config, _get_image, bdm_get_by_instance_uuid, get_flavor, instance_save, - req_spec_get_by_inst_uuid): + req_spec_get_by_inst_uuid, req_spec_save): orig_system_metadata = {} get_flavor.return_value = test_flavor.fake_flavor orig_image_href = 'orig_image' @@ -3241,8 +3242,15 @@ class _ComputeAPIUnitTestMixIn(object): injected_files=files_to_inject, image_ref=new_image_href, orig_image_ref=orig_image_href, orig_sys_metadata=orig_system_metadata, bdms=bdms, - preserve_ephemeral=False, host=instance.host, + preserve_ephemeral=False, host=None, request_spec=fake_spec, kwargs={}) + # assert the request spec was modified so the scheduler picks + # the existing instance host/node + req_spec_save.assert_called_once_with() + self.assertIn('requested_destination', fake_spec) + requested_destination = fake_spec.requested_destination + self.assertEqual(instance.host, requested_destination.host) + self.assertEqual(instance.node, requested_destination.node) _check_auto_disk_config.assert_called_once_with(image=new_image) _checks_for_create_and_rebuild.assert_called_once_with(self.context, diff --git a/nova/tests/unit/conductor/test_conductor.py b/nova/tests/unit/conductor/test_conductor.py index 481f5aa259e6..b93071558265 100644 --- a/nova/tests/unit/conductor/test_conductor.py +++ b/nova/tests/unit/conductor/test_conductor.py @@ -1361,7 +1361,10 @@ class _BaseTaskTestCase(object): self.conductor_manager.rebuild_instance(context=self.context, instance=inst_obj, **rebuild_args) - reset_fd.assert_called_once_with() + if rebuild_args['recreate']: + reset_fd.assert_called_once_with() + else: + reset_fd.assert_not_called() select_dest_mock.assert_called_once_with(self.context, fake_spec, [inst_obj.uuid]) compute_args['host'] = expected_host