Merge "Ensure source compute is up when confirming a resize"

Author: Zuul

nova/api/openstack/compute/servers.py

@@ -885,7 +885,10 @@ def _action_confirm_resize(self, req, id, body):
         except exception.MigrationNotFound:
             msg = _("Instance has not been resized.")
             raise exc.HTTPBadRequest(explanation=msg)
-        except exception.InstanceIsLocked as e:
+        except (
+            exception.InstanceIsLocked,
+            exception.ServiceUnavailable,
+        ) as e:
             raise exc.HTTPConflict(explanation=e.format_message())
         except exception.InstanceInvalidState as state_error:
             common.raise_http_conflict_for_instance_invalid_state(state_error,

nova/compute/api.py

@@ -3775,10 +3775,40 @@ def revert_resize(self, context, instance):
                                               migration.dest_compute,
                                               reqspec)
 
+    @staticmethod
+    def _get_source_compute_service(context, migration):
+        """Find the source compute Service object given the Migration.
+
+        :param context: nova auth RequestContext target at the destination
+            compute cell
+        :param migration: Migration object for the move operation
+        :return: Service object representing the source host nova-compute
+        """
+        if migration.cross_cell_move:
+            # The source compute could be in another cell so look up the
+            # HostMapping to determine the source cell.
+            hm = objects.HostMapping.get_by_host(
+                context, migration.source_compute)
+            with nova_context.target_cell(context, hm.cell_mapping) as cctxt:
+                return objects.Service.get_by_compute_host(
+                    cctxt, migration.source_compute)
+        # Same-cell migration so just use the context we have.
+        return objects.Service.get_by_compute_host(
+            context, migration.source_compute)
+
     @check_instance_lock
     @check_instance_state(vm_state=[vm_states.RESIZED])
     def confirm_resize(self, context, instance, migration=None):
-        """Confirms a migration/resize and deletes the 'old' instance."""
+        """Confirms a migration/resize and deletes the 'old' instance.
+
+        :param context: nova auth RequestContext
+        :param instance: Instance object to confirm the resize
+        :param migration: Migration object; provided if called from the
+            _poll_unconfirmed_resizes periodic task on the dest compute.
+        :raises: MigrationNotFound if migration is not provided and a migration
+            cannot be found for the instance with status "finished".
+        :raises: ServiceUnavailable if the source compute service is down.
+        """
         elevated = context.elevated()
         # NOTE(melwitt): We're not checking quota here because there isn't a
         # change in resource usage when confirming a resize. Resource
@@ -3789,6 +3819,13 @@ def confirm_resize(self, context, instance, migration=None):
             migration = objects.Migration.get_by_instance_and_status(
                 elevated, instance.uuid, 'finished')
 
+        # Check if the source compute service is up before modifying the
+        # migration record because once we do we cannot come back through this
+        # method since it will be looking for a "finished" status migration.
+        source_svc = self._get_source_compute_service(context, migration)
+        if not self.servicegroup_api.service_is_up(source_svc):
+            raise exception.ServiceUnavailable()
+
         migration.status = 'confirming'
         migration.save()
 

nova/tests/fixtures.py

@@ -324,8 +324,12 @@ def _fake_cell_list(self, *args):
 
     @contextmanager
     def _fake_target_cell(self, context, target_cell):
-        # NOTE(danms): Just pass through the context without actually
-        # targeting anything.
+        # Just do something simple and set/unset the cell_uuid on the context.
+        if target_cell:
+            context.cell_uuid = getattr(target_cell, 'uuid',
+                                        uuidsentinel.cell1)
+        else:
+            context.cell_uuid = None
         yield context
 
     def _fake_set_target_cell(self, context, cell_mapping):

nova/tests/functional/test_cross_cell_migrate.py

@@ -963,6 +963,10 @@ def test_anti_affinity_group(self):
             server2['id'], {'migrate': {'host': 'host2'}})
         self._wait_for_migration_status(server2, ['error'])
 
+    @mock.patch('nova.compute.api.API._get_source_compute_service',
+        new=mock.Mock())
+    @mock.patch('nova.servicegroup.api.API.service_is_up',
+        new=mock.Mock(return_value=True))
     def test_poll_unconfirmed_resizes_with_upcall(self):
         """Tests the _poll_unconfirmed_resizes periodic task with a cross-cell
         resize once the instance is in VERIFY_RESIZE status on the dest host.
@@ -993,6 +997,10 @@ def test_poll_unconfirmed_resizes_with_upcall(self):
         self._assert_confirm(
             server, source_rp_uuid, target_rp_uuid, new_flavor)
 
+    @mock.patch('nova.compute.api.API._get_source_compute_service',
+        new=mock.Mock())
+    @mock.patch('nova.servicegroup.api.API.service_is_up',
+        new=mock.Mock(return_value=True))
     def test_poll_unconfirmed_resizes_with_no_upcall(self):
         """Tests the _poll_unconfirmed_resizes periodic task with a cross-cell
         resize once the instance is in VERIFY_RESIZE status on the dest host.

nova/tests/functional/test_servers.py

@@ -46,7 +46,6 @@
 from nova.tests.functional.api import client
 from nova.tests.functional import integrated_helpers
 from nova.tests.unit.api.openstack import fakes
-from nova.tests.unit import cast_as_call
 from nova.tests.unit import fake_block_device
 from nova.tests.unit import fake_notifier
 from nova.tests.unit import fake_requests
@@ -3359,34 +3358,37 @@ def test_source_host_down_during_confirm(self):
         self.api.put_service(
             source_service['id'], {'status': 'disabled', 'forced_down': True})
         # Now configure auto-confirm and call the method on the target compute
-        # so we do not have to wait for the periodic to run. Also configure
-        # the RPC call from the API to the source compute to timeout after 1
-        # second.
-        self.flags(resize_confirm_window=1, rpc_response_timeout=1)
+        # so we do not have to wait for the periodic to run.
+        self.flags(resize_confirm_window=1)
         # Stub timeutils so the DB API query finds the unconfirmed migration.
         future = timeutils.utcnow() + datetime.timedelta(hours=1)
         ctxt = context.get_admin_context()
         with osloutils_fixture.TimeFixture(future):
-            # Use the CastAsCall fixture since the fake rpc is not going to
-            # simulate a failure from the service being down.
-            with cast_as_call.CastAsCall(self):
-                self.computes['host2'].manager._poll_unconfirmed_resizes(ctxt)
+            self.computes['host2'].manager._poll_unconfirmed_resizes(ctxt)
         self.assertIn('Error auto-confirming resize',
                       self.stdlog.logger.output)
-        self.assertIn('No reply on topic compute', self.stdlog.logger.output)
-        # FIXME(mriedem): This is bug 1855927 where the migration status is
-        # left in "confirming" so the _poll_unconfirmed_resizes task will not
-        # process it again nor can the user confirm the resize in the API since
-        # the migration status is not "finished".
-        self._wait_for_migration_status(server, ['confirming'])
+        self.assertIn('Service is unavailable at this time',
+                      self.stdlog.logger.output)
+        # The source compute service check should have been done before the
+        # migration status was updated so it should still be "finished".
+        self._wait_for_migration_status(server, ['finished'])
+        # Try to confirm in the API while the source compute service is still
+        # down to assert the 409 (rather than a 500) error.
         ex = self.assertRaises(client.OpenStackApiException,
                                self.api.post_server_action,
                                server['id'], {'confirmResize': None})
-        self.assertEqual(400, ex.response.status_code)
-        self.assertIn('Instance has not been resized', six.text_type(ex))
-        # That error message is bogus because the server is resized.
-        server = self.api.get_server(server['id'])
-        self.assertEqual('VERIFY_RESIZE', server['status'])
+        self.assertEqual(409, ex.response.status_code)
+        self.assertIn('Service is unavailable at this time', six.text_type(ex))
+        # Bring the source compute back up and try to confirm the resize which
+        # should work since the migration status is still "finished".
+        self.restart_compute_service(self.computes['host1'])
+        self.api.put_service(
+            source_service['id'], {'status': 'enabled', 'forced_down': False})
+        # Use the API to confirm the resize because _poll_unconfirmed_resizes
+        # requires mucking with the current time which causes problems with
+        # the service_is_up check in the API.
+        self.api.post_server_action(server['id'], {'confirmResize': None})
+        self._wait_for_state_change(server, 'ACTIVE')
 
 
 class ServerLiveMigrateForceAndAbort(

nova/tests/unit/compute/test_api.py

@@ -1644,10 +1644,13 @@ def test(mock_inst_get, mock_map_get):
 
         test()
 
+    @mock.patch('nova.compute.api.API._get_source_compute_service')
+    @mock.patch('nova.servicegroup.api.API.service_is_up', return_value=True)
     @mock.patch.object(objects.Migration, 'save')
     @mock.patch.object(objects.Migration, 'get_by_instance_and_status')
     @mock.patch.object(context.RequestContext, 'elevated')
     def _test_confirm_resize(self, mock_elevated, mock_get, mock_save,
+                             mock_service_is_up, mock_get_service,
                              mig_ref_passed=False):
         params = dict(vm_state=vm_states.RESIZED)
         fake_inst = self._create_instance_obj(params=params)
@@ -1678,6 +1681,8 @@ def _check_mig(expected_task_state=None):
             self.compute_api.confirm_resize(self.context, fake_inst)
 
         mock_elevated.assert_called_once_with()
+        mock_service_is_up.assert_called_once_with(
+            mock_get_service.return_value)
         mock_save.assert_called_once_with()
         mock_record.assert_called_once_with(self.context, fake_inst,
                                             'confirmResize')
@@ -1694,6 +1699,34 @@ def test_confirm_resize(self):
     def test_confirm_resize_with_migration_ref(self):
         self._test_confirm_resize(mig_ref_passed=True)
 
+    @mock.patch('nova.objects.HostMapping.get_by_host',
+                return_value=objects.HostMapping(
+                    cell_mapping=objects.CellMapping(
+                        database_connection='fake://', transport_url='none://',
+                        uuid=uuids.cell_uuid)))
+    @mock.patch('nova.objects.Service.get_by_compute_host')
+    def test_get_source_compute_service(self, mock_service_get, mock_hm_get):
+        # First start with a same-cell migration.
+        migration = objects.Migration(source_compute='source.host',
+                                      cross_cell_move=False)
+        self.compute_api._get_source_compute_service(self.context, migration)
+        mock_hm_get.assert_not_called()
+        mock_service_get.assert_called_once_with(self.context, 'source.host')
+        # Make sure the context was not targeted.
+        ctxt = mock_service_get.call_args[0][0]
+        self.assertIsNone(ctxt.cell_uuid)
+
+        # Now test with a cross-cell migration.
+        mock_service_get.reset_mock()
+        migration.cross_cell_move = True
+        self.compute_api._get_source_compute_service(self.context, migration)
+        mock_hm_get.assert_called_once_with(self.context, 'source.host')
+        mock_service_get.assert_called_once_with(
+            test.MatchType(context.RequestContext), 'source.host')
+        # Make sure the context was targeted.
+        ctxt = mock_service_get.call_args[0][0]
+        self.assertEqual(uuids.cell_uuid, ctxt.cell_uuid)
+
     @mock.patch('nova.virt.hardware.numa_get_constraints')
     @mock.patch('nova.network.neutron.API.get_requested_resource_for_instance',
                 return_value=[])
@@ -7460,8 +7493,10 @@ def test_get_migrations_sorted_filter_duplicates_using_created_at(self):
         self._test_get_migrations_sorted_filter_duplicates(
             [older, newer], newer)
 
+    @mock.patch('nova.servicegroup.api.API.service_is_up', return_value=True)
     @mock.patch('nova.objects.Migration.get_by_instance_and_status')
-    def test_confirm_resize_cross_cell_move_true(self, mock_migration_get):
+    def test_confirm_resize_cross_cell_move_true(self, mock_migration_get,
+                                                 mock_service_is_up):
         """Tests confirm_resize where Migration.cross_cell_move is True"""
         instance = fake_instance.fake_instance_obj(
             self.context, vm_state=vm_states.RESIZED, task_state=None,
@@ -7475,12 +7510,15 @@ def test_confirm_resize_cross_cell_move_true(self, mock_migration_get):
             mock.patch.object(self.compute_api, '_record_action_start'),
             mock.patch.object(self.compute_api.compute_task_api,
                               'confirm_snapshot_based_resize'),
+            mock.patch.object(self.compute_api, '_get_source_compute_service'),
         ) as (
             mock_elevated, mock_migration_save, mock_record_action,
-            mock_conductor_confirm
+            mock_conductor_confirm, mock_get_service
         ):
             self.compute_api.confirm_resize(self.context, instance)
         mock_elevated.assert_called_once_with()
+        mock_service_is_up.assert_called_once_with(
+            mock_get_service.return_value)
         mock_migration_save.assert_called_once_with()
         self.assertEqual('confirming', migration.status)
         mock_record_action.assert_called_once_with(

nova/tests/unit/scheduler/test_host_manager.py

@@ -219,7 +219,8 @@ def test_init_instance_info_compute_nodes(self, mock_get_all,
         inst2 = objects.Instance(host='host1', uuid=uuids.instance_2)
         inst3 = objects.Instance(host='host2', uuid=uuids.instance_3)
         cell = objects.CellMapping(database_connection='',
-                                   target_url='')
+                                   target_url='',
+                                   uuid=uuids.cell_uuid)
         mock_get_by_filters.return_value = objects.InstanceList(
                 objects=[inst1, inst2, inst3])
         hm = self.host_manager
@@ -589,7 +590,7 @@ def test_get_host_states(self, mock_get_by_host, mock_get_all,
         mock_get_by_host.return_value = []
         mock_get_all.return_value = fakes.COMPUTE_NODES
         mock_get_by_binary.return_value = fakes.SERVICES
-        context = 'fake_context'
+        context = nova_context.get_admin_context()
         compute_nodes, services = self.host_manager._get_computes_for_cells(
             context, self.host_manager.enabled_cells)
 
@@ -786,7 +787,7 @@ def test_host_state_update(self, mock_get_by_host):
 
     @mock.patch('nova.objects.InstanceList.get_uuids_by_host')
     def test_host_state_not_updated(self, mock_get_by_host):
-        context = 'fake_context'
+        context = nova_context.get_admin_context()
         hm = self.host_manager
         inst1 = objects.Instance(uuid=uuids.instance)
         cn1 = objects.ComputeNode(host='host1')
@@ -806,10 +807,11 @@ def test_host_state_not_updated(self, mock_get_by_host):
 
     @mock.patch('nova.objects.InstanceList.get_uuids_by_host')
     def test_recreate_instance_info(self, mock_get_by_host):
+        context = nova_context.get_admin_context()
         host_name = 'fake_host'
-        inst1 = fake_instance.fake_instance_obj('fake_context',
+        inst1 = fake_instance.fake_instance_obj(context,
                                                 uuid=uuids.instance_1)
-        inst2 = fake_instance.fake_instance_obj('fake_context',
+        inst2 = fake_instance.fake_instance_obj(context,
                                                 uuid=uuids.instance_2)
         orig_inst_dict = {inst1.uuid: inst1, inst2.uuid: inst2}
         mock_get_by_host.return_value = [uuids.instance_1, uuids.instance_2]
@@ -818,7 +820,7 @@ def test_recreate_instance_info(self, mock_get_by_host):
                     'instances': orig_inst_dict,
                     'updated': True,
                 }}
-        self.host_manager._recreate_instance_info('fake_context', host_name)
+        self.host_manager._recreate_instance_info(context, host_name)
         new_info = self.host_manager._instance_info[host_name]
         self.assertEqual(len(new_info['instances']),
                          len(mock_get_by_host.return_value))
@@ -1231,7 +1233,7 @@ def test_get_host_states(self, mock_get_by_host, mock_get_all,
         mock_get_by_host.return_value = []
         mock_get_all.return_value = fakes.COMPUTE_NODES
         mock_get_by_binary.return_value = fakes.SERVICES
-        context = 'fake_context'
+        context = nova_context.get_admin_context()
 
         compute_nodes, services = self.host_manager._get_computes_for_cells(
             context, self.host_manager.enabled_cells)
@@ -1256,7 +1258,7 @@ def test_get_host_states_after_delete_one(self, mock_get_by_host,
         mock_get_by_host.return_value = []
         mock_get_all.side_effect = [fakes.COMPUTE_NODES, running_nodes]
         mock_get_by_binary.side_effect = [fakes.SERVICES, fakes.SERVICES]
-        context = 'fake_context'
+        context = nova_context.get_admin_context()
 
         # first call: all nodes
         compute_nodes, services = self.host_manager._get_computes_for_cells(
@@ -1286,7 +1288,7 @@ def test_get_host_states_after_delete_all(self, mock_get_by_host,
         mock_get_by_host.return_value = []
         mock_get_all.side_effect = [fakes.COMPUTE_NODES, []]
         mock_get_by_binary.side_effect = [fakes.SERVICES, fakes.SERVICES]
-        context = 'fake_context'
+        context = nova_context.get_admin_context()
 
         # first call: all nodes
         compute_nodes, services = self.host_manager._get_computes_for_cells(