openstack
/
nova
Code Issues Proposed changes

Use provider mappings from Placement (mostly) 

fill_provider_mapping is used from *most* code paths where it's
necessary to associate RequestSpec.request_groups with the resource
providers that are satisfying them. (Specifically, all the code paths
where we have a Selection object available. More about that below.)

Prior to Placement microversion 1.34, the only way to do this mapping
was by reproducing much of the logic from GET /allocation_candidates
locally to reverse engineer the associations. This was incomplete,
imperfect, inefficient, and ugly. That workaround was nested in the call
from fill_provider_mapping to fill_provider_mapping_based_on_allocation.

Placement microversion 1.34 enhanced GET /allocation_candidates to
return these mappings [1], and Nova started using 1.34 as of [2], so
this commit makes fill_provider_mapping bypass
fill_provider_mapping_based_on_allocations completely.

We would love to get rid of the entire hack, but
fill_provider_mapping_based_on_allocation is still used from
finish_revert_resize to restore port bindings on a reverted migration.
And when reverting a migration, we don't have allocation candidates with
mappings, only the original source allocations. It is left to a future
patch to figure out how to get around this, conceivably by saving the
original mappings in the migration context.

[1] https://docs.openstack.org/placement/train/specs/train/implemented/placement-resource-provider-request-group-mapping-in-allocation-candidates.html
[2] I52499ff6639c1a5815a8557b22dd33106dcc386b

Related to blueprint: placement-resource-provider-request-group-mapping-in-allocation-candidates
Change-Id: I45e0b2b73f88b86a20bc70ddf4f9bb97c8ea8312
This commit is contained in:
Eric Fried 2019-12-02 18:01:25 -06:00
parent 2e3985c303
commit 7daa3f59e2
6 changed files with 109 additions and 120 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

9
nova/conductor/manager.py
View File

@ -729,8 +729,7 @@ class ComputeTaskManager(base.Base):
# moves the allocation of the instance to
# another host
scheduler_utils.fill_provider_mapping(
context, self.report_client, request_spec,
host)
request_spec, host)
except Exception as exc:
self._cleanup_when_reschedule_fails(
context, instance, exc, legacy_request_spec,
@ -1144,8 +1143,7 @@ class ComputeTaskManager(base.Base):
if recreate:
scheduler_utils.fill_provider_mapping(
context, self.report_client, request_spec,
selection)
request_spec, selection)
except (exception.NoValidHost,
exception.UnsupportedPolicyException,
@ -1531,8 +1529,7 @@ class ComputeTaskManager(base.Base):
# allocations in the scheduler) for this instance, we may need to
# map allocations to resource providers in the request spec.
try:
scheduler_utils.fill_provider_mapping(
context, self.report_client, request_spec, host)
scheduler_utils.fill_provider_mapping(request_spec, host)
except Exception as exc:
# If anything failed here we need to cleanup and bail out.
with excutils.save_and_reraise_exception():

6
nova/conductor/tasks/migrate.py
View File

@ -441,8 +441,7 @@ class MigrationTask(base.TaskBase):
selection, self.host_list = self._get_host_supporting_request(
selection_list)
scheduler_utils.fill_provider_mapping(
self.context, self.reportclient, self.request_spec, selection)
scheduler_utils.fill_provider_mapping(self.request_spec, selection)
return selection
def _reschedule(self):
@ -479,8 +478,7 @@ class MigrationTask(base.TaskBase):
selection.allocation_request_version)
if host_available:
scheduler_utils.fill_provider_mapping(
self.context, self.reportclient, self.request_spec,
selection)
self.request_spec, selection)
else:
# Some deployments use different schedulers that do not
# use Placement, so they will not have an

38
nova/scheduler/utils.py
View File

@ -1214,23 +1214,10 @@ def get_weight_multiplier(host_state, multiplier_name, multiplier_config):
return value
def fill_provider_mapping(
context, report_client, request_spec, host_selection):
def fill_provider_mapping(request_spec, host_selection):
"""Fills out the request group - resource provider mapping in the
request spec.
This is a workaround as placement does not return which RP
fulfills which granular request group in the allocation candidate
request. There is a spec proposing a solution in placement:
https://review.opendev.org/#/c/597601/
When that spec is implemented then this function can be
replaced with a simpler code that copies the group - RP
mapping out from the Selection object returned by the scheduler's
select_destinations call.
:param context: The security context
:param report_client: SchedulerReportClient instance to be used to
communicate with placement
:param request_spec: The RequestSpec object associated with the
operation
:param host_selection: The Selection object returned by the scheduler
@ -1244,11 +1231,19 @@ def fill_provider_mapping(
# allocations in placement but if request_spec.maps_requested_resources
# is not empty and the scheduling succeeded then placement has to be
# involved
ar = jsonutils.loads(host_selection.allocation_request)
allocs = ar['allocations']
mappings = jsonutils.loads(host_selection.allocation_request)['mappings']
fill_provider_mapping_based_on_allocation(
context, report_client, request_spec, allocs)
for request_group in request_spec.requested_resources:
# NOTE(efried): We can count on request_group.requester_id being set:
# - For groups from flavors, ResourceRequest.get_request_group sets it
# to the group suffix.
# - For groups from other sources (e.g. ports, accelerators), it is
# required to be set by ResourceRequest._add_request_group, and that
# method uses it as the suffix.
# And we can count on mappings[requester_id] existing because each
# RequestGroup translated into a (replete - empties are disallowed by
# ResourceRequest._add_request_group) group fed to Placement.
request_group.provider_uuids = mappings[request_group.requester_id]
def fill_provider_mapping_based_on_allocation(
@ -1262,6 +1257,13 @@ def fill_provider_mapping_based_on_allocation(
the mapping is calculated based on the allocation of the source host the
move operation is reverting to.
This is a workaround as placement does not return which RP fulfills which
granular request group except in the allocation candidate request (because
request groups are ephemeral, only existing in the scope of that request).
.. todo:: Figure out a better way to preserve the mappings so we can get
rid of this workaround.
:param context: The security context
:param report_client: SchedulerReportClient instance to be used to
communicate with placement

38
nova/tests/unit/conductor/tasks/test_migrate.py
View File

@ -125,15 +125,9 @@ class MigrationTaskTestCase(test.NoDBTestCase):
cn_mock.side_effect = set_migration_uuid
selection = self.host_lists[0][0]
with test.nested(
mock.patch.object(scheduler_utils,
'fill_provider_mapping'),
mock.patch.object(task, '_is_selected_host_in_source_cell',
return_value=same_cell)
) as (fill_mock, _is_source_cell_mock):
with mock.patch.object(task, '_is_selected_host_in_source_cell',
return_value=same_cell) as _is_source_cell_mock:
task.execute()
fill_mock.assert_called_once_with(
task.context, task.reportclient, task.request_spec, selection)
_is_source_cell_mock.assert_called_once_with(selection)
self.ensure_network_metadata_mock.assert_called_once_with(
@ -253,11 +247,9 @@ class MigrationTaskTestCase(test.NoDBTestCase):
mock_get_resources.assert_called_once_with(
self.context, self.instance.uuid)
@mock.patch.object(scheduler_utils, 'fill_provider_mapping')
@mock.patch.object(scheduler_utils, 'claim_resources')
@mock.patch.object(context.RequestContext, 'elevated')
def test_execute_reschedule(
self, mock_elevated, mock_claim, mock_fill_provider_mapping):
def test_execute_reschedule(self, mock_elevated, mock_claim):
report_client = report.SchedulerReportClient()
# setup task for re-schedule
alloc_req = {
@ -286,9 +278,6 @@ class MigrationTaskTestCase(test.NoDBTestCase):
mock_claim.assert_called_once_with(
mock_elevated.return_value, report_client, self.request_spec,
self.instance.uuid, alloc_req, '1.19')
mock_fill_provider_mapping.assert_called_once_with(
self.context, report_client, self.request_spec,
alternate_selection)
@mock.patch.object(scheduler_utils, 'fill_provider_mapping')
@mock.patch.object(scheduler_utils, 'claim_resources')
@ -724,14 +713,12 @@ class MigrationTaskTestCase(test.NoDBTestCase):
])
@mock.patch.object(migrate.LOG, 'debug')
@mock.patch('nova.scheduler.utils.fill_provider_mapping')
@mock.patch('nova.scheduler.utils.claim_resources')
@mock.patch('nova.objects.Service.get_by_host_and_binary')
def test_reschedule_old_compute_skipped(
self, mock_get_service, mock_claim_resources, mock_fill_mapping,
mock_debug):
self, mock_get_service, mock_claim_resources, mock_debug):
self.request_spec.requested_resources = [
objects.RequestGroup()
objects.RequestGroup(requester_id=uuids.port1)
]
task = self._generate_task()
resources = {
@ -745,15 +732,17 @@ class MigrationTaskTestCase(test.NoDBTestCase):
nodename="node1",
cell_uuid=uuids.cell1,
allocation_request=jsonutils.dumps(
{"allocations": {uuids.host1: resources}}),
allocation_request_version='1.19')
{"allocations": {uuids.host1: resources},
"mappings": {uuids.port1: [uuids.host1]}}),
allocation_request_version='1.34')
second = objects.Selection(
service_host="host2",
nodename="node2",
cell_uuid=uuids.cell1,
allocation_request=jsonutils.dumps(
{"allocations": {uuids.host2: resources}}),
allocation_request_version='1.19')
{"allocations": {uuids.host2: resources},
"mappings": {uuids.port1: [uuids.host2]}}),
allocation_request_version='1.34')
first_service = objects.Service(service_host='host1')
first_service.version = 38
@ -775,9 +764,8 @@ class MigrationTaskTestCase(test.NoDBTestCase):
mock_claim_resources.assert_called_once_with(
self.context.elevated(), task.reportclient, task.request_spec,
self.instance.uuid,
{"allocations": {uuids.host2: resources}}, '1.19')
mock_fill_mapping.assert_called_once_with(
task.context, task.reportclient, task.request_spec, second)
{"allocations": {uuids.host2: resources},
"mappings": {uuids.port1: [uuids.host2]}}, '1.34')
mock_debug.assert_has_calls([
mock.call(
'Scheduler returned alternate host %(host)s as a possible '

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

@ -48,7 +48,6 @@ from nova.objects import base as obj_base
from nova.objects import block_device as block_device_obj
from nova.objects import fields
from nova.scheduler.client import query
from nova.scheduler.client import report
from nova.scheduler import utils as scheduler_utils
from nova import test
from nova.tests import fixtures
@ -68,24 +67,42 @@ from nova.volume import cinder
CONF = conf.CONF
fake_alloc1 = {"allocations": {
uuids.host1: {
"resources": {"VCPU": 1,
"MEMORY_MB": 1024,
"DISK_GB": 100}
}}}
fake_alloc2 = {"allocations": {
uuids.host2: {
"resources": {"VCPU": 1,
"MEMORY_MB": 1024,
"DISK_GB": 100}
}}}
fake_alloc3 = {"allocations": {
uuids.host3: {
"resources": {"VCPU": 1,
"MEMORY_MB": 1024,
"DISK_GB": 100}
}}}
fake_alloc1 = {
"allocations": {
uuids.host1: {
"resources": {"VCPU": 1,
"MEMORY_MB": 1024,
"DISK_GB": 100}
}
},
"mappings": {
uuids.port1: [uuids.host1]
}
}
fake_alloc2 = {
"allocations": {
uuids.host2: {
"resources": {"VCPU": 1,
"MEMORY_MB": 1024,
"DISK_GB": 100}
}
},
"mappings": {
uuids.port1: [uuids.host2]
}
}
fake_alloc3 = {
"allocations": {
uuids.host3: {
"resources": {"VCPU": 1,
"MEMORY_MB": 1024,
"DISK_GB": 100}
}
},
"mappings": {
uuids.port1: [uuids.host3]
}
}
fake_alloc_json1 = jsonutils.dumps(fake_alloc1)
fake_alloc_json2 = jsonutils.dumps(fake_alloc2)
fake_alloc_json3 = jsonutils.dumps(fake_alloc3)
@ -1028,11 +1045,9 @@ class _BaseTaskTestCase(object):
host_list=expected_build_run_host_list)
mock_rp_mapping.assert_called_once_with(
self.context,
test.MatchType(report.SchedulerReportClient),
test.MatchType(objects.RequestSpec),
test.MatchType(objects.Selection))
actual_request_spec = mock_rp_mapping.mock_calls[0][1][2]
actual_request_spec = mock_rp_mapping.mock_calls[0][1][0]
self.assertEqual(
rg1.resources,
actual_request_spec.requested_resources[0].resources)
@ -1113,11 +1128,9 @@ class _BaseTaskTestCase(object):
# called only once when the claim succeeded
mock_rp_mapping.assert_called_once_with(
self.context,
test.MatchType(report.SchedulerReportClient),
test.MatchType(objects.RequestSpec),
test.MatchType(objects.Selection))
actual_request_spec = mock_rp_mapping.mock_calls[0][1][2]
actual_request_spec = mock_rp_mapping.mock_calls[0][1][0]
self.assertEqual(
rg1.resources,
actual_request_spec.requested_resources[0].resources)
@ -1750,7 +1763,6 @@ class _BaseTaskTestCase(object):
get_req_res_mock.assert_called_once_with(
self.context, inst_obj.uuid)
fill_rp_mapping_mock.assert_called_once_with(
self.context, self.conductor_manager.report_client,
fake_spec, fake_selection)
self.assertEqual('compute.instance.rebuild.scheduled',
@ -2291,24 +2303,13 @@ class ConductorTaskTestCase(_BaseTaskTestCase, test_compute.BaseTestCase):
self.assertTrue(mock_build.called)
@mock.patch('nova.scheduler.client.report.SchedulerReportClient.'
'get_provider_traits')
@mock.patch('nova.objects.request_spec.RequestSpec.'
'map_requested_resources_to_providers')
def test_schedule_and_build_instances_fill_request_spec(
self, mock_map, mock_traits):
def test_schedule_and_build_instances_fill_request_spec(self):
# makes sure there is some request group in the spec to be mapped
self.params['request_specs'][0].requested_resources = [
objects.RequestGroup()]
mock_traits.return_value.traits = ['TRAIT1']
objects.RequestGroup(requester_id=uuids.port1)]
self._do_schedule_and_build_instances_test(self.params)
mock_map.assert_called_once_with(fake_alloc1['allocations'],
{uuids.host1: ['TRAIT1']})
mock_traits.assert_called_once_with(mock.ANY, uuids.host1)
@mock.patch('nova.conductor.manager.ComputeTaskManager.'
'_cleanup_build_artifacts')
@mock.patch('nova.scheduler.utils.'

59
nova/tests/unit/scheduler/test_utils.py
View File

@ -1371,45 +1371,48 @@ class TestUtils(TestUtilsBase):
utils.get_weight_multiplier(host1, 'cpu_weight_multiplier', 1.0)
)
@mock.patch('nova.scheduler.utils.'
'fill_provider_mapping_based_on_allocation')
def test_fill_provider_mapping_returns_early_if_nothing_to_do(
self, mock_fill_provider):
context = nova_context.RequestContext()
def _set_up_and_fill_provider_mapping(self, requested_resources):
request_spec = objects.RequestSpec()
# set up the request that there is nothing to do
request_spec.requested_resources = []
report_client = mock.sentinel.report_client
selection = objects.Selection()
utils.fill_provider_mapping(
context, report_client, request_spec, selection)
mock_fill_provider.assert_not_called()
@mock.patch('nova.scheduler.utils.'
'fill_provider_mapping_based_on_allocation')
def test_fill_provider_mapping(self, mock_fill_provider):
context = nova_context.RequestContext()
request_spec = objects.RequestSpec()
request_spec.requested_resources = [objects.RequestGroup()]
report_client = mock.sentinel.report_client
request_spec.requested_resources = requested_resources
allocs = {
uuids.rp_uuid: {
uuids.rp_uuid1: {
'resources': {
'NET_BW_EGR_KILOBIT_PER_SEC': 1,
}
},
uuids.rp_uuid2: {
'resources': {
'NET_BW_INGR_KILOBIT_PER_SEC': 1,
}
}
}
allocation_req = {'allocations': allocs}
mappings = {
uuids.port_id1: [uuids.rp_uuid2],
uuids.port_id2: [uuids.rp_uuid1],
}
allocation_req = {'allocations': allocs, 'mappings': mappings}
selection = objects.Selection(
allocation_request=jsonutils.dumps(allocation_req))
utils.fill_provider_mapping(
context, report_client, request_spec, selection)
# Unmapped initially
for rg in requested_resources:
self.assertEqual([], rg.provider_uuids)
mock_fill_provider.assert_called_once_with(
context, report_client, request_spec, allocs)
utils.fill_provider_mapping(request_spec, selection)
def test_fill_provider_mapping(self):
rg1 = objects.RequestGroup(requester_id=uuids.port_id1)
rg2 = objects.RequestGroup(requester_id=uuids.port_id2)
self._set_up_and_fill_provider_mapping([rg1, rg2])
# Validate the mappings
self.assertEqual([uuids.rp_uuid2], rg1.provider_uuids)
self.assertEqual([uuids.rp_uuid1], rg2.provider_uuids)
def test_fill_provider_mapping_no_op(self):
# This just proves that having 'mappings' in the allocation request
# doesn't break anything.
self._set_up_and_fill_provider_mapping([])
@mock.patch.object(objects.RequestSpec,
'map_requested_resources_to_providers')