openstack
/
nova
Code Issues Proposed changes

Pass RequestSpec to ConductorTaskAPI.build_instances 

There are two things that call the conductor task API's
build_instances method:

1. nova-api when using cellsv1
2. nova-compute when rescheduling during a build failure

Since we want to eventually replace the filter_properties
usage in the compute service with the request_spec, this
allows passing the request_spec from the compute service
during a reschedule.

For now we have to continue munging that RequestSpec a
bit using the filter_properties for retries and the
next chosen host from the scheduler, but that can eventually
go away. With this, the computes can start to at least
pull scheduler_hints out of the request_spec rather than
the filter_properties, which is one more step in removing
the compute's dependency on filter_properties.

Note that we don't pass the request_spec from nova-api
for cells v1, simply because cells v1 is deprecated.

Part of blueprint request-spec-use-by-compute

Change-Id: Ie5233bd481013413f12e55201588d37a9688ae78
This commit is contained in:
Matt Riedemann 2017-10-26 13:49:25 -04:00
parent 609ddc2244
commit eb889886e2
7 changed files with 150 additions and 46 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

5
nova/compute/manager.py
View File

@ -1896,13 +1896,10 @@ class ComputeManager(manager.Manager):
instance.task_state = task_states.SCHEDULING
instance.save()
# TODO(mriedem): Pass the request_spec back to conductor so that
# it gets to the next chosen host during the reschedule and we
# can hopefully eventually get rid of the legacy filter_properties.
self.compute_task_api.build_instances(context, [instance],
image, filter_properties, admin_password,
injected_files, requested_networks, security_groups,
block_device_mapping)
block_device_mapping, request_spec=request_spec)
return build_results.RESCHEDULED
except (exception.InstanceNotFound,
exception.UnexpectedDeletingTaskStateError):

5
nova/conductor/api.py
View File

@ -112,7 +112,8 @@ class ComputeTaskAPI(object):
def build_instances(self, context, instances, image, filter_properties,
admin_password, injected_files, requested_networks,
security_groups, block_device_mapping, legacy_bdm=True):
security_groups, block_device_mapping, legacy_bdm=True,
request_spec=None):
self.conductor_compute_rpcapi.build_instances(context,
instances=instances, image=image,
filter_properties=filter_properties,
@ -120,7 +121,7 @@ class ComputeTaskAPI(object):
requested_networks=requested_networks,
security_groups=security_groups,
block_device_mapping=block_device_mapping,
legacy_bdm=legacy_bdm)
legacy_bdm=legacy_bdm, request_spec=request_spec)
def schedule_and_build_instances(self, context, build_requests,
request_spec, image,

31
nova/conductor/manager.py
View File

@ -218,7 +218,7 @@ class ComputeTaskManager(base.Base):
may involve coordinating activities on multiple compute nodes.
"""
target = messaging.Target(namespace='compute_task', version='1.17')
target = messaging.Target(namespace='compute_task', version='1.18')
def __init__(self):
super(ComputeTaskManager, self).__init__()
@ -515,7 +515,8 @@ class ComputeTaskManager(base.Base):
# (which go to the cell conductor and thus are always cell-specific).
def build_instances(self, context, instances, image, filter_properties,
admin_password, injected_files, requested_networks,
security_groups, block_device_mapping=None, legacy_bdm=True):
security_groups, block_device_mapping=None, legacy_bdm=True,
request_spec=None):
# TODO(ndipanov): Remove block_device_mapping and legacy_bdm in version
# 2.0 of the RPC API.
# TODO(danms): Remove this in version 2.0 of the RPC API
@ -532,14 +533,27 @@ class ComputeTaskManager(base.Base):
flavor = objects.Flavor.get_by_id(context, flavor['id'])
filter_properties = dict(filter_properties, instance_type=flavor)
request_spec = {}
# Older computes will not send a request_spec during reschedules, nor
# will the API send the request_spec if using cells v1, so we need
# to check and build our own if one is not provided.
if request_spec is None:
request_spec = scheduler_utils.build_request_spec(
image, instances)
else:
# TODO(mriedem): This is annoying but to populate the local
# request spec below using the filter_properties, we have to pass
# in a primitive version of the request spec. Yes it's inefficient
# and we can remove it once the populate_retry and
# populate_filter_properties utility methods are converted to
# work on a RequestSpec object rather than filter_properties.
request_spec = request_spec.to_legacy_request_spec_dict()
try:
# check retry policy. Rather ugly use of instances[0]...
# but if we've exceeded max retries... then we really only
# have a single instance.
# TODO(sbauza): Provide directly the RequestSpec object
# when populate_retry() accepts it
request_spec = scheduler_utils.build_request_spec(image, instances)
scheduler_utils.populate_retry(
filter_properties, instances[0].uuid)
instance_uuids = [instance.uuid for instance in instances]
@ -579,6 +593,13 @@ class ComputeTaskManager(base.Base):
local_filter_props = copy.deepcopy(filter_properties)
scheduler_utils.populate_filter_properties(local_filter_props,
host)
# Populate the request_spec with the local_filter_props information
# like retries and limits. Note that at this point the request_spec
# could have come from a compute via reschedule and it would
# already have some things set, like scheduler_hints.
local_reqspec = objects.RequestSpec.from_primitives(
context, request_spec, local_filter_props)
# The block_device_mapping passed from the api doesn't contain
# instance specific information
bdms = objects.BlockDeviceMappingList.get_by_instance_uuid(
@ -607,7 +628,7 @@ class ComputeTaskManager(base.Base):
self.compute_rpcapi.build_and_run_instance(context,
instance=instance, host=host.service_host, image=image,
request_spec=request_spec,
request_spec=local_reqspec,
filter_properties=local_filter_props,
admin_password=admin_password,
injected_files=injected_files,

12
nova/conductor/rpcapi.py
View File

@ -274,6 +274,7 @@ class ComputeTaskAPI(object):
1.15 - Added live_migrate_instance
1.16 - Added schedule_and_build_instances
1.17 - Added tags to schedule_and_build_instances()
1.18 - Added request_spec to build_instances().
"""
def __init__(self):
@ -327,9 +328,14 @@ class ComputeTaskAPI(object):
def build_instances(self, context, instances, image, filter_properties,
admin_password, injected_files, requested_networks,
security_groups, block_device_mapping, legacy_bdm=True):
security_groups, block_device_mapping, legacy_bdm=True,
request_spec=None):
image_p = jsonutils.to_primitive(image)
version = '1.10'
version = '1.18'
send_reqspec = True
if not self.client.can_send_version(version):
send_reqspec = False
version = '1.10'
if not self.client.can_send_version(version):
version = '1.9'
if 'instance_type' in filter_properties:
@ -343,6 +349,8 @@ class ComputeTaskAPI(object):
'injected_files': injected_files,
'requested_networks': requested_networks,
'security_groups': security_groups}
if send_reqspec:
kw['request_spec'] = request_spec
if not self.client.can_send_version(version):
version = '1.8'
kw['requested_networks'] = kw['requested_networks'].as_tuples()

6
nova/tests/functional/test_compute_mgr.py
View File

@ -70,8 +70,10 @@ class ComputeManagerTestCase(test.TestCase):
mock_spawn.side_effect = test.TestingException('Preserve this')
# Simulate that we're on the last retry attempt
filter_properties = {'retry': {'num_attempts': 3}}
request_spec = objects.RequestSpec.from_primitives(
self.context, {}, filter_properties)
self.compute.manager.build_and_run_instance(
self.context, instance, {}, {}, filter_properties,
block_device_mapping=[])
self.context, instance, {}, request_spec,
filter_properties, block_device_mapping=[])
_test()
self.assertIn('Preserve this', instance.fault.message)

18
nova/tests/unit/compute/test_compute_mgr.py
View File

@ -4497,7 +4497,8 @@ class ComputeManagerBuildInstanceTestCase(test.NoDBTestCase):
mock_build.assert_called_once_with(self.context,
[self.instance], self.image, self.filter_properties,
self.admin_pass, self.injected_files, self.requested_networks,
self.security_groups, self.block_device_mapping)
self.security_groups, self.block_device_mapping,
request_spec={})
@mock.patch.object(manager.ComputeManager, '_shutdown_instance')
@mock.patch.object(manager.ComputeManager, '_build_networks_for_instance')
@ -4581,7 +4582,8 @@ class ComputeManagerBuildInstanceTestCase(test.NoDBTestCase):
mock_build_ins.assert_called_once_with(self.context,
[instance], self.image, self.filter_properties,
self.admin_pass, self.injected_files, self.requested_networks,
self.security_groups, self.block_device_mapping)
self.security_groups, self.block_device_mapping,
request_spec={})
@mock.patch.object(manager.ComputeManager, '_build_and_run_instance')
@mock.patch.object(conductor_api.ComputeTaskAPI, 'build_instances')
@ -4636,7 +4638,8 @@ class ComputeManagerBuildInstanceTestCase(test.NoDBTestCase):
mock_build_ins.assert_called_once_with(self.context,
[instance], self.image, self.filter_properties,
self.admin_pass, self.injected_files, self.requested_networks,
self.security_groups, self.block_device_mapping)
self.security_groups, self.block_device_mapping,
request_spec={})
@mock.patch.object(objects.InstanceActionEvent,
'event_finish_with_failure')
@ -4736,7 +4739,8 @@ class ComputeManagerBuildInstanceTestCase(test.NoDBTestCase):
mock_build.assert_called_once_with(self.context,
[self.instance], self.image, self.filter_properties,
self.admin_pass, self.injected_files, self.requested_networks,
self.security_groups, self.block_device_mapping)
self.security_groups, self.block_device_mapping,
request_spec={})
@mock.patch.object(objects.InstanceActionEvent,
'event_finish_with_failure')
@ -4784,7 +4788,8 @@ class ComputeManagerBuildInstanceTestCase(test.NoDBTestCase):
mock_build.assert_called_once_with(self.context,
[self.instance], self.image, self.filter_properties,
self.admin_pass, self.injected_files, self.requested_networks,
self.security_groups, self.block_device_mapping)
self.security_groups, self.block_device_mapping,
request_spec={})
@mock.patch.object(objects.InstanceActionEvent,
'event_finish_with_failure')
@ -5171,7 +5176,8 @@ class ComputeManagerBuildInstanceTestCase(test.NoDBTestCase):
mock_build.assert_called_once_with(self.context, [self.instance],
self.image, self.filter_properties, self.admin_pass,
self.injected_files, self.requested_networks,
self.security_groups, self.block_device_mapping)
self.security_groups, self.block_device_mapping,
request_spec={})
mock_nil.assert_called_once_with(self.instance)
mock_clean.assert_called_once_with(self.context, self.instance,
self.compute.host)

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

@ -394,6 +394,9 @@ class _BaseTaskTestCase(object):
@mock.patch.object(objects.RequestSpec, 'from_primitives')
def test_build_instances(self, mock_fp, mock_save, mock_getaz,
mock_buildreq):
"""Tests creating two instances and the scheduler returns a unique
host/node combo for each instance.
"""
fake_spec = objects.RequestSpec
mock_fp.return_value = fake_spec
instance_type = flavors.get_default_flavor()
@ -424,19 +427,16 @@ class _BaseTaskTestCase(object):
).AndReturn([[fake_selection1], [fake_selection2]])
db.block_device_mapping_get_all_by_instance(self.context,
instances[0].uuid).AndReturn([])
filter_properties2 = {'retry': {'num_attempts': 1,
'hosts': [['host1', 'node1']]},
'limits': {}}
self.conductor_manager.compute_rpcapi.build_and_run_instance(
self.context,
instance=mox.IgnoreArg(),
host='host1',
image={'fake_data': 'should_pass_silently'},
request_spec={
'image': {'fake_data': 'should_pass_silently'},
'instance_properties': instance_properties,
'instance_type': instance_type_p,
'num_instances': 2},
filter_properties={'retry': {'num_attempts': 1,
'hosts': [['host1', 'node1']]},
'limits': {}},
request_spec=fake_spec,
filter_properties=filter_properties2,
admin_password='admin_password',
injected_files='injected_files',
requested_networks=None,
@ -445,19 +445,16 @@ class _BaseTaskTestCase(object):
node='node1', limits=None)
db.block_device_mapping_get_all_by_instance(self.context,
instances[1].uuid).AndReturn([])
filter_properties3 = {'limits': {},
'retry': {'num_attempts': 1,
'hosts': [['host2', 'node2']]}}
self.conductor_manager.compute_rpcapi.build_and_run_instance(
self.context,
instance=mox.IgnoreArg(),
host='host2',
image={'fake_data': 'should_pass_silently'},
request_spec={
'image': {'fake_data': 'should_pass_silently'},
'instance_properties': instance_properties,
'instance_type': instance_type_p,
'num_instances': 2},
filter_properties={'limits': {},
'retry': {'num_attempts': 1,
'hosts': [['host2', 'node2']]}},
request_spec=fake_spec,
filter_properties=filter_properties3,
admin_password='admin_password',
injected_files='injected_files',
requested_networks=None,
@ -484,7 +481,12 @@ class _BaseTaskTestCase(object):
mock_getaz.assert_has_calls([
mock.call(self.context, 'host1'),
mock.call(self.context, 'host2')])
mock_fp.assert_called_once_with(self.context, spec, filter_properties)
# A RequestSpec is built from primitives once before calling the
# scheduler to get hosts and then once per instance we're building.
mock_fp.assert_has_calls([
mock.call(self.context, spec, filter_properties),
mock.call(self.context, spec, filter_properties2),
mock.call(self.context, spec, filter_properties3)])
@mock.patch.object(scheduler_utils, 'build_request_spec')
@mock.patch.object(scheduler_utils, 'setup_instance_group')
@ -916,7 +918,7 @@ class _BaseTaskTestCase(object):
filter_properties = fake_spec.to_legacy_filter_properties_dict()
request_spec = fake_spec.to_legacy_request_spec_dict()
host = {'host': 'host1', 'nodename': 'node1', 'limits': []}
host = {'host': 'host1', 'nodename': 'node1', 'limits': {}}
# unshelve_instance() is a cast, we need to wait for it to complete
self.useFixture(cast_as_call.CastAsCall(self))
@ -2389,13 +2391,14 @@ class ConductorTaskTestCase(_BaseTaskTestCase, test_compute.BaseTestCase):
instances[0].save().AndRaise(
exc.InstanceNotFound(instance_id=instances[0].uuid))
instances[1].save()
filter_properties2 = {'limits': {},
'retry': {'num_attempts': 1,
'hosts': [['host2', 'node2']]}}
self.conductor_manager.compute_rpcapi.build_and_run_instance(
self.context, instance=instances[1], host='host2',
image={'fake-data': 'should_pass_silently'}, request_spec=spec,
filter_properties={'limits': {},
'retry': {'num_attempts': 1,
'hosts': [['host2',
'node2']]}},
image={'fake-data': 'should_pass_silently'},
request_spec=fake_spec,
filter_properties=filter_properties2,
admin_password='admin_password',
injected_files='injected_files',
requested_networks=None,
@ -2417,7 +2420,12 @@ class ConductorTaskTestCase(_BaseTaskTestCase, test_compute.BaseTestCase):
security_groups='security_groups',
block_device_mapping='block_device_mapping',
legacy_bdm=False)
fp.assert_called_once_with(self.context, spec, filter_properties)
# RequestSpec.from_primitives is called once before we call the
# scheduler to select_destinations and then once per instance that
# gets build in the compute.
fp.assert_has_calls([
mock.call(self.context, spec, filter_properties),
mock.call(self.context, spec, filter_properties2)])
@mock.patch.object(scheduler_utils, 'setup_instance_group')
@mock.patch.object(scheduler_utils, 'build_request_spec')
@ -2466,7 +2474,8 @@ class ConductorTaskTestCase(_BaseTaskTestCase, test_compute.BaseTestCase):
get_buildreq.return_value.destroy.assert_called_once_with()
build_and_run_instance.assert_called_once_with(self.context,
instance=instances[1], host='host2', image={'fake-data':
'should_pass_silently'}, request_spec=spec,
'should_pass_silently'},
request_spec=from_primitives.return_value,
filter_properties={'limits': {},
'retry': {'num_attempts': 1,
'hosts': [['host2',
@ -2754,6 +2763,66 @@ class ConductorTaskRPCAPITestCase(_BaseTaskTestCase,
self.context, 'schedule_and_build_instances', **kw)
_test()
def test_build_instances_with_request_spec_ok(self):
"""Tests passing a request_spec to the build_instances RPC API
method and having it passed through to the conductor task manager.
"""
image = {}
cctxt_mock = mock.MagicMock()
@mock.patch.object(self.conductor.client, 'can_send_version',
return_value=True)
@mock.patch.object(self.conductor.client, 'prepare',
return_value=cctxt_mock)
def _test(prepare_mock, can_send_mock):
self.conductor.build_instances(
self.context, mock.sentinel.instances, image,
mock.sentinel.filter_properties, mock.sentinel.admin_password,
mock.sentinel.injected_files, mock.sentinel.requested_networks,
mock.sentinel.security_groups,
mock.sentinel.block_device_mapping,
request_spec=mock.sentinel.request_spec)
kw = {'instances': mock.sentinel.instances, 'image': image,
'filter_properties': mock.sentinel.filter_properties,
'admin_password': mock.sentinel.admin_password,
'injected_files': mock.sentinel.injected_files,
'requested_networks': mock.sentinel.requested_networks,
'security_groups': mock.sentinel.security_groups,
'request_spec': mock.sentinel.request_spec}
cctxt_mock.cast.assert_called_once_with(
self.context, 'build_instances', **kw)
_test()
def test_build_instances_with_request_spec_cannot_send(self):
"""Tests passing a request_spec to the build_instances RPC API
method but not having it passed through to the conductor task manager
because the version is too old to handle it.
"""
image = {}
cctxt_mock = mock.MagicMock()
@mock.patch.object(self.conductor.client, 'can_send_version',
side_effect=(False, True, True, True))
@mock.patch.object(self.conductor.client, 'prepare',
return_value=cctxt_mock)
def _test(prepare_mock, can_send_mock):
self.conductor.build_instances(
self.context, mock.sentinel.instances, image,
mock.sentinel.filter_properties, mock.sentinel.admin_password,
mock.sentinel.injected_files, mock.sentinel.requested_networks,
mock.sentinel.security_groups,
mock.sentinel.block_device_mapping,
request_spec=mock.sentinel.request_spec)
kw = {'instances': mock.sentinel.instances, 'image': image,
'filter_properties': mock.sentinel.filter_properties,
'admin_password': mock.sentinel.admin_password,
'injected_files': mock.sentinel.injected_files,
'requested_networks': mock.sentinel.requested_networks,
'security_groups': mock.sentinel.security_groups}
cctxt_mock.cast.assert_called_once_with(
self.context, 'build_instances', **kw)
_test()
class ConductorTaskAPITestCase(_BaseTaskTestCase, test_compute.BaseTestCase):
"""Compute task API Tests."""