openstack/nova
Code Issues Proposed changes

Validate new image via scheduler during rebuild

Browse Source 
During a rebuild we bypass the scheduler because we are
always rebuilding the instance on the same host it's already
on. However, we allow passing a new image during rebuild
and that new image needs to be validated to work with the
instance host by running it through the scheduler filters,
like the ImagePropertiesFilter. Otherwise the new image
could violate constraints placed on the host by the admin.

This change checks to see if there is a new image provided
and if so, modifies the request spec passed to the scheduler
so that the new image is validated all while restricting
the scheduler to still pick the same host that the instance
is running on. If the image is not valid for the host, the
scheduler will raise NoValidHost and the rebuild stops.

A functional test is added to show the recreate of the bug
and that we probably stop the rebuild now in conductor by
calling the scheduler to validate the image.

Co-Authored-By: Sylvain Bauza <sbauza@redhat.com>

Closes-Bug: #1664931

Change-Id: I11746d1ea996a0f18b7c54b4c9c21df58cc4714b
This commit is contained in:
Matt Riedemann 2017-10-27 16:03:15 -04:00
parent 50c23d8c86
commit 984dd8ad6a
7 changed files with 117 additions and 13 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

17
nova/compute/api.py
View File

@ -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)

16
nova/conductor/manager.py
View File

@ -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 []

10
nova/tests/functional/api/client.py
View File

@ -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']

8
nova/tests/functional/integrated_helpers.py
View File

@ -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'])

62
nova/tests/functional/test_servers.py
View File

@ -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

12
nova/tests/unit/compute/test_compute_api.py
View File

@ -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,

3
nova/tests/unit/conductor/test_conductor.py
View File

@ -1361,7 +1361,10 @@ class _BaseTaskTestCase(object):
self.conductor_manager.rebuild_instance(context=self.context,
instance=inst_obj,
**rebuild_args)
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