Only call _fill_provider_mapping if claim succeeds

During re-schedule condutor takes the next Selecton object from the
host list and tries to allocate the requested resources on the host in
the Selection object. So far the conductor also tried to find the
resource provide mapping for such allocation even if the resource claim
is failed. This is unnecessary. This patch makes sure that mapping is
tried to be calculated if the claim succeeds first.

Change-Id: I9944398c38d11466d27c2a4b24035b26d264b000
Closes-Bug: #1822262

Author: Balazs Gibizer

nova/conductor/manager.py

@@ -686,7 +686,7 @@ def build_instances(self, context, instances, image, filter_properties,
                                 elevated, self.report_client, spec_obj,
                                 instance.uuid, alloc_req,
                                 host.allocation_request_version)
-                            if request_spec:
+                            if request_spec and host_available:
                                 # NOTE(gibi): redo the request group - resource
                                 # provider mapping as the above claim call
                                 # moves the allocation of the instance to

nova/tests/unit/conductor/test_conductor.py

@@ -1027,6 +1027,89 @@ def do_test(mock_destroy_build_req, mock_pop_inst_map,
 
         do_test()
 
+    @mock.patch.object(objects.Instance, 'save', new=mock.MagicMock())
+    @mock.patch.object(query.SchedulerQueryClient, 'select_destinations')
+    @mock.patch.object(conductor_manager.ComputeTaskManager,
+                       '_set_vm_state_and_notify', new=mock.MagicMock())
+    def test_build_instances_reschedule_not_recalc_mapping_if_claim_fails(
+            self, mock_select_dests):
+
+        rg1 = objects.RequestGroup(resources={"CUSTOM_FOO": 1})
+        request_spec = objects.RequestSpec(requested_resources=[rg1])
+
+        mock_select_dests.return_value = [[fake_selection1]]
+        instance = fake_instance.fake_instance_obj(self.context)
+        image = {'fake-data': 'should_pass_silently'}
+
+        # build_instances() is a cast, we need to wait for it to complete
+        self.useFixture(cast_as_call.CastAsCall(self))
+
+        @mock.patch('nova.conductor.manager.ComputeTaskManager.'
+                    '_fill_provider_mapping')
+        @mock.patch('nova.scheduler.utils.claim_resources',
+                    # simulate that the first claim fails during re-schedule
+                    side_effect=[False, True])
+        @mock.patch('nova.objects.request_spec.RequestSpec.from_primitives',
+                    return_value=request_spec)
+        @mock.patch.object(self.conductor_manager.compute_rpcapi,
+                           'build_and_run_instance')
+        @mock.patch.object(self.conductor_manager,
+                           '_populate_instance_mapping')
+        @mock.patch.object(self.conductor_manager, '_destroy_build_request')
+        def do_test(mock_destroy_build_req, mock_pop_inst_map,
+                    mock_build_and_run, mock_request_spec_from_primitives,
+                    mock_claim, mock_rp_mapping):
+            self.conductor.build_instances(
+                context=self.context,
+                instances=[instance],
+                image=image,
+                filter_properties={'retry': {'num_attempts': 1, 'hosts': []}},
+                admin_password='admin_password',
+                injected_files='injected_files',
+                requested_networks=None,
+                security_groups='security_groups',
+                block_device_mapping='block_device_mapping',
+                legacy_bdm=False,
+                host_lists=copy.deepcopy(fake_host_lists_alt),
+                request_spec=request_spec)
+
+            expected_build_run_host_list = copy.copy(fake_host_lists_alt[0])
+            if expected_build_run_host_list:
+                # first is consumed but the claim fails so the conductor takes
+                # the next host
+                expected_build_run_host_list.pop(0)
+                # second is consumed and claim succeeds
+                expected_build_run_host_list.pop(0)
+            mock_build_and_run.assert_called_with(
+                self.context,
+                instance=mock.ANY,
+                host='host2',
+                image=image,
+                request_spec=request_spec,
+                filter_properties={'retry': {'num_attempts': 2,
+                                             'hosts': [['host2', 'node2']]},
+                                   'limits': {}},
+                admin_password='admin_password',
+                injected_files='injected_files',
+                requested_networks=None,
+                security_groups='security_groups',
+                block_device_mapping=test.MatchType(
+                    objects.BlockDeviceMappingList),
+                node='node2',
+                limits=None,
+                host_list=expected_build_run_host_list)
+
+            # called only once when the claim succeeded
+            mock_rp_mapping.assert_called_once_with(
+                self.context, instance.uuid, mock.ANY,
+                test.MatchType(objects.Selection))
+            actual_request_spec = mock_rp_mapping.mock_calls[0][1][2]
+            self.assertEqual(
+                rg1.resources,
+                actual_request_spec.requested_resources[0].resources)
+
+        do_test()
+
     @mock.patch.object(cinder.API, 'attachment_get')
     @mock.patch.object(cinder.API, 'attachment_create')
     @mock.patch.object(block_device_obj.BlockDeviceMapping, 'save')