From 1b2deeb7e679e1bdf2fe9769bcc2f33fdd87fb30 Mon Sep 17 00:00:00 2001 From: Matt Riedemann Date: Thu, 26 Oct 2017 12:16:00 -0400 Subject: [PATCH] Use the RequestSpec when getting scheduler_hints in compute For the first chosen host during an instance build, conductor will pass both filter_properties and request_spec and they both have scheduler_hints. We want to eventually get rid of the legacy filter_properties, so this change uses the request_spec, if available, to get the scheduler_hints in the compute during the build. These are only used when validating server group (anti-)affinity policies. If the request_spec isn't available, because we rescheduled to another host for example, then we fallback to the filter_properties to get the hints. Part of blueprint request-spec-use-by-compute Change-Id: I49ffebcd129990f1835f404d98b51732a32171eb --- nova/compute/manager.py | 41 +++++++++++-- nova/tests/unit/compute/test_compute_mgr.py | 67 ++++++++++++++++----- 2 files changed, 86 insertions(+), 22 deletions(-) diff --git a/nova/compute/manager.py b/nova/compute/manager.py index 3ea26bf970..6fca1ce5ee 100644 --- a/nova/compute/manager.py +++ b/nova/compute/manager.py @@ -1284,7 +1284,7 @@ class ComputeManager(manager.Manager): return [_decode(f) for f in injected_files] def _validate_instance_group_policy(self, context, instance, - filter_properties): + scheduler_hints): # NOTE(russellb) Instance group policy is enforced by the scheduler. # However, there is a race condition with the enforcement of # the policy. Since more than one instance may be scheduled at the @@ -1293,12 +1293,16 @@ class ComputeManager(manager.Manager): # multiple instances with an affinity policy could end up on different # hosts. This is a validation step to make sure that starting the # instance here doesn't violate the policy. - - scheduler_hints = filter_properties.get('scheduler_hints') or {} group_hint = scheduler_hints.get('group') if not group_hint: return + # The RequestSpec stores scheduler_hints as key=list pairs so we need + # to check the type on the value and pull the single entry out. The + # API request schema validates that the 'group' hint is a single value. + if isinstance(group_hint, list): + group_hint = group_hint[0] + @utils.synchronized(group_hint) def _do_validation(context, instance, group_hint): group = objects.InstanceGroup.get_by_hint(context, group_hint) @@ -1844,7 +1848,7 @@ class ComputeManager(manager.Manager): self._build_and_run_instance(context, instance, image, decoded_files, admin_password, requested_networks, security_groups, block_device_mapping, node, limits, - filter_properties) + filter_properties, request_spec) LOG.info('Took %0.2f seconds to build instance.', timer.elapsed(), instance=instance) return build_results.ACTIVE @@ -1893,6 +1897,9 @@ 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, @@ -1950,9 +1957,29 @@ class ComputeManager(manager.Manager): return True return False + @staticmethod + def _get_scheduler_hints(filter_properties, request_spec=None): + """Helper method to get scheduler hints. + + This method prefers to get the hints out of the request spec, but that + might not be provided. Conductor will pass request_spec down to the + first compute chosen for a build but older computes will not pass + the request_spec to conductor's build_instances method for a + a reschedule, so if we're on a host via a retry, request_spec may not + be provided so we need to fallback to use the filter_properties + to get scheduler hints. + """ + hints = {} + if request_spec is not None and 'scheduler_hints' in request_spec: + hints = request_spec.scheduler_hints + if not hints: + hints = filter_properties.get('scheduler_hints') or {} + return hints + def _build_and_run_instance(self, context, instance, image, injected_files, admin_password, requested_networks, security_groups, - block_device_mapping, node, limits, filter_properties): + block_device_mapping, node, limits, filter_properties, + request_spec=None): image_name = image.get('name') self._notify_about_instance_usage(context, instance, 'create.start', @@ -1970,13 +1997,15 @@ class ComputeManager(manager.Manager): self._check_device_tagging(requested_networks, block_device_mapping) try: + scheduler_hints = self._get_scheduler_hints(filter_properties, + request_spec) rt = self._get_resource_tracker() with rt.instance_claim(context, instance, node, limits): # NOTE(russellb) It's important that this validation be done # *after* the resource tracker instance claim, as that is where # the host is set on the instance. self._validate_instance_group_policy(context, instance, - filter_properties) + scheduler_hints) image_meta = objects.ImageMeta.from_dict(image) with self._build_resources(context, instance, requested_networks, security_groups, image_meta, diff --git a/nova/tests/unit/compute/test_compute_mgr.py b/nova/tests/unit/compute/test_compute_mgr.py index 5953015a43..a5778baedc 100644 --- a/nova/tests/unit/compute/test_compute_mgr.py +++ b/nova/tests/unit/compute/test_compute_mgr.py @@ -4193,6 +4193,31 @@ class ComputeManagerUnitTestCase(test.NoDBTestCase): mock.call(self.context, inst_obj, 'fake-mini', action='soft_delete', phase='end')]) + def test_get_scheduler_hints(self): + # 1. No hints and no request_spec. + self.assertEqual({}, self.compute._get_scheduler_hints({})) + # 2. Hints come from the filter_properties. + hints = {'foo': 'bar'} + filter_properties = {'scheduler_hints': hints} + self.assertEqual( + hints, self.compute._get_scheduler_hints(filter_properties)) + # 3. Hints come from filter_properties because reqspec is empty. + reqspec = objects.RequestSpec.from_primitives(self.context, {}, {}) + self.assertEqual( + hints, self.compute._get_scheduler_hints( + filter_properties, reqspec)) + # 4. Hints come from the request spec. + reqspec_hints = {'boo': 'baz'} + reqspec = objects.RequestSpec.from_primitives( + self.context, {}, {'scheduler_hints': reqspec_hints}) + # The RequestSpec unconditionally stores hints as a key=list + # unlike filter_properties which just stores whatever came in from + # the API request. + expected_reqspec_hints = {'boo': ['baz']} + self.assertDictEqual( + expected_reqspec_hints, self.compute._get_scheduler_hints( + filter_properties, reqspec)) + class ComputeManagerBuildInstanceTestCase(test.NoDBTestCase): def setUp(self): @@ -4312,7 +4337,7 @@ class ComputeManagerBuildInstanceTestCase(test.NoDBTestCase): self.image, self.injected_files, self.admin_pass, self.requested_networks, self.security_groups, self.block_device_mapping, self.node, self.limits, - self.filter_properties) + self.filter_properties, {}) # This test when sending an icehouse compatible rpc call to juno compute # node, NetworkRequest object can load from three items tuple. @@ -4378,7 +4403,7 @@ class ComputeManagerBuildInstanceTestCase(test.NoDBTestCase): self.image, self.injected_files, self.admin_pass, self.requested_networks, self.security_groups, self.block_device_mapping, self.node, self.limits, - self.filter_properties) + self.filter_properties, {}) mock_clean_net.assert_called_once_with(self.context, self.instance, self.requested_networks) mock_clean_vol.assert_called_once_with(self.context, @@ -4427,7 +4452,7 @@ class ComputeManagerBuildInstanceTestCase(test.NoDBTestCase): self.image, self.injected_files, self.admin_pass, self.requested_networks, self.security_groups, self.block_device_mapping, self.node, self.limits, - self.filter_properties) + self.filter_properties, {}) mock_clean.assert_called_once_with(self.context, self.instance, self.compute.host) mock_nil.assert_called_once_with(self.instance) @@ -4512,7 +4537,7 @@ class ComputeManagerBuildInstanceTestCase(test.NoDBTestCase): self.image, self.injected_files, self.admin_pass, self.requested_networks, self.security_groups, self.block_device_mapping, self.node, self.limits, - self.filter_properties) + self.filter_properties, {}) mock_cleanup_network.assert_called_once_with( self.context, instance, self.compute.host) mock_build_ins.assert_called_once_with(self.context, @@ -4567,7 +4592,7 @@ class ComputeManagerBuildInstanceTestCase(test.NoDBTestCase): self.image, self.injected_files, self.admin_pass, self.requested_networks, self.security_groups, self.block_device_mapping, self.node, self.limits, - self.filter_properties) + self.filter_properties, {}) mock_cleanup_network.assert_called_once_with( self.context, instance, self.requested_networks) mock_build_ins.assert_called_once_with(self.context, @@ -4611,7 +4636,7 @@ class ComputeManagerBuildInstanceTestCase(test.NoDBTestCase): mock_build_run.assert_called_once_with(self.context, self.instance, self.image, self.injected_files, self.admin_pass, self.requested_networks, self.security_groups, - self.block_device_mapping, self.node, self.limits, {}) + self.block_device_mapping, self.node, self.limits, {}, {}) mock_clean_net.assert_called_once_with(self.context, self.instance, self.requested_networks) mock_clean_vol.assert_called_once_with(self.context, @@ -4665,7 +4690,7 @@ class ComputeManagerBuildInstanceTestCase(test.NoDBTestCase): self.image, self.injected_files, self.admin_pass, self.requested_networks, self.security_groups, self.block_device_mapping, self.node, self.limits, - self.filter_properties) + self.filter_properties, {}) mock_deallocate.assert_called_once_with(self.instance) mock_clean_inst.assert_called_once_with(self.context, self.instance, self.compute.host) @@ -4713,7 +4738,7 @@ class ComputeManagerBuildInstanceTestCase(test.NoDBTestCase): self.image, self.injected_files, self.admin_pass, self.requested_networks, self.security_groups, self.block_device_mapping, self.node, self.limits, - self.filter_properties) + self.filter_properties, {}) mock_deallocate.assert_called_once_with(self.instance) mock_clean.assert_called_once_with(self.context, self.instance, self.requested_networks) @@ -4773,7 +4798,7 @@ class ComputeManagerBuildInstanceTestCase(test.NoDBTestCase): self.image, self.injected_files, self.admin_pass, self.requested_networks, self.security_groups, self.block_device_mapping, self.node, self.limits, - self.filter_properties) + self.filter_properties, {}) mock_clean_net.assert_called_once_with(self.context, self.instance, self.requested_networks) @@ -5034,7 +5059,7 @@ class ComputeManagerBuildInstanceTestCase(test.NoDBTestCase): self.limits, self.filter_properties) _validate_instance_group_policy.assert_called_once_with( - self.context, self.instance, self.filter_properties) + self.context, self.instance, {}) _build_networks_for_instance.assert_has_calls( [mock.call(self.context, self.instance, self.requested_networks, self.security_groups)]) @@ -5188,23 +5213,33 @@ class ComputeManagerBuildInstanceTestCase(test.NoDBTestCase): @mock.patch('nova.objects.InstanceGroup.get_by_hint') def test_validate_policy_honors_workaround_disabled(self, mock_get): instance = objects.Instance(uuid=uuids.instance) - filter_props = {'scheduler_hints': {'group': 'foo'}} + hints = {'group': 'foo'} mock_get.return_value = objects.InstanceGroup(policies=[]) self.compute._validate_instance_group_policy(self.context, - instance, - filter_props) + instance, hints) mock_get.assert_called_once_with(self.context, 'foo') @mock.patch('nova.objects.InstanceGroup.get_by_hint') def test_validate_policy_honors_workaround_enabled(self, mock_get): self.flags(disable_group_policy_check_upcall=True, group='workarounds') instance = objects.Instance(uuid=uuids.instance) - filter_props = {'scheduler_hints': {'group': 'foo'}} + hints = {'group': 'foo'} self.compute._validate_instance_group_policy(self.context, - instance, - filter_props) + instance, hints) self.assertFalse(mock_get.called) + @mock.patch('nova.objects.InstanceGroup.get_by_hint') + def test_validate_instance_group_policy_handles_hint_list(self, mock_get): + """Tests that _validate_instance_group_policy handles getting + scheduler_hints from a RequestSpec which stores the hints as a key=list + pair. + """ + instance = objects.Instance(uuid=uuids.instance) + hints = {'group': [uuids.group_hint]} + self.compute._validate_instance_group_policy(self.context, + instance, hints) + mock_get.assert_called_once_with(self.context, uuids.group_hint) + def test_failed_bdm_prep_from_delete_raises_unexpected(self): with test.nested( mock.patch.object(self.compute,