Remove pre-cellsv2 short circuit in instance get

This removes a couple short circuits around instance cell mapping,
which were in place for pre-cellsv2 code. Now, they defeat some of
our atomic logic around what should be done in and around deletes.
These were TODOs to remove once people had to be on cellsv2, which
is now the case.

Note that the api samples tests were depending on these in order to
work at all (which is a good demonstration of why they need to
be removed). This patch also extends the SingleCellSimple fixture
with a couple more mocks to make them happily take the same path
as the rest of the code. Since the default in that fixture is
now to make instances look mapped all the time, we also need a flag
so the tests that check for yet-to-be-mapped instances can still
function.

Related-Bug: #1660878
Change-Id: I7eb4b41fd2f33e8a63e329adbafc47d149e31552

nova/compute/api.py

@@ -2293,23 +2293,9 @@ class API(base.Base):
                             context, instance_uuid,
                             expected_attrs=expected_attrs)
                 else:
-                    # If BuildRequest is not found but inst_map.cell_mapping
-                    # does not point at a cell then cell migration has not
-                    # happened yet. This will be a failure case later.
-                    # TODO(alaski): Make this a failure case after we put in
-                    # a block that requires migrating to cellsv2.
-                    instance = objects.Instance.get_by_uuid(
-                        context, instance_uuid, expected_attrs=expected_attrs)
+                    raise exception.InstanceNotFound(instance_id=instance_uuid)
         else:
-            # This should not happen once a deployment has migrated to cellsv2.
-            # If it happens after that point we handle it gracefully for now
-            # but this will become an exception in the future.
-            # TODO(alaski): Once devstack is setting up cellsv2 by default add
-            # a warning log message that this will become an exception in the
-            # future. The warning message will be conditional upon the
-            # migration having happened, which means a db lookup to check that.
-            instance = objects.Instance.get_by_uuid(
-                context, instance_uuid, expected_attrs=expected_attrs)
+            raise exception.InstanceNotFound(instance_id=instance_uuid)
 
         return instance
 

nova/tests/fixtures.py

@@ -264,8 +264,14 @@ class SingleCellSimple(fixtures.Fixture):
 
     If you need to distinguish between cell0 and cellN, then you
     should use the CellDatabases fixture.
+
+    If instances should appear to still be in scheduling state, pass
+    instances_created=False to init.
     """
 
+    def __init__(self, instances_created=True):
+        self.instances_created = instances_created
+
     def setUp(self):
         super(SingleCellSimple, self).setUp()
         self.useFixture(fixtures.MonkeyPatch(
@@ -277,6 +283,12 @@ class SingleCellSimple(fixtures.Fixture):
         self.useFixture(fixtures.MonkeyPatch(
             'nova.objects.HostMapping._get_by_host_from_db',
             self._fake_hostmapping_get))
+        self.useFixture(fixtures.MonkeyPatch(
+            'nova.objects.InstanceMapping._get_by_instance_uuid_from_db',
+            self._fake_instancemapping_get))
+        self.useFixture(fixtures.MonkeyPatch(
+            'nova.objects.InstanceMapping._save_in_db',
+            self._fake_instancemapping_get))
         self.useFixture(fixtures.MonkeyPatch(
             'nova.context.target_cell',
             self._fake_target_cell))
@@ -288,6 +300,18 @@ class SingleCellSimple(fixtures.Fixture):
                 'host': 'host1',
                 'cell_mapping': self._fake_cell_list()[0]}
 
+    def _fake_instancemapping_get(self, *args):
+        return {
+            'id': 1,
+            'updated_at': None,
+            'created_at': None,
+            'instance_uuid': uuidsentinel.instance,
+            'cell_id': (self.instances_created and 1 or None),
+            'project_id': 'project',
+            'cell_mapping': (
+                self.instances_created and self._fake_cell_get() or None),
+        }
+
     def _fake_cell_get(self, *args):
         return self._fake_cell_list()[0]
 

nova/tests/functional/wsgi/test_servers.py

@@ -44,7 +44,8 @@ class ServersPreSchedulingTestCase(test.TestCase):
 
         self.api = api_fixture.api
         self.api.microversion = 'latest'
-        self.useFixture(nova_fixtures.SingleCellSimple())
+        self.useFixture(nova_fixtures.SingleCellSimple(
+            instances_created=False))
 
     def test_instance_from_buildrequest(self):
         self.useFixture(nova_fixtures.AllServicesCurrent())

nova/tests/unit/compute/test_compute_api.py

@@ -4223,19 +4223,21 @@ class _ComputeAPIUnitTestMixIn(object):
             mock_get_inst_map):
 
         self.useFixture(fixtures.AllServicesCurrent())
-        # Just check that an InstanceMappingNotFound causes the instance to
-        # get looked up normally.
-        self.compute_api.get(self.context, uuids.inst_uuid)
-        mock_get_build_req.assert_not_called()
         if self.cell_type is None:
-            mock_get_inst_map.assert_called_once_with(self.context,
-                                                      uuids.inst_uuid)
-        mock_get_inst.assert_called_once_with(self.context, uuids.inst_uuid,
-                                              expected_attrs=[
-                                                  'metadata',
-                                                  'system_metadata',
-                                                  'security_groups',
-                                                  'info_cache'])
+            # No Mapping means NotFound
+            self.assertRaises(exception.InstanceNotFound,
+                              self.compute_api.get, self.context,
+                              uuids.inst_uuid)
+        else:
+            self.compute_api.get(self.context, uuids.inst_uuid)
+            mock_get_build_req.assert_not_called()
+            mock_get_inst.assert_called_once_with(self.context,
+                                                  uuids.inst_uuid,
+                                                  expected_attrs=[
+                                                      'metadata',
+                                                      'system_metadata',
+                                                      'security_groups',
+                                                      'info_cache'])
 
     @mock.patch.object(objects.Service, 'get_minimum_version', return_value=15)
     @mock.patch.object(objects.InstanceMapping, 'get_by_instance_uuid')
@@ -4336,23 +4338,21 @@ class _ComputeAPIUnitTestMixIn(object):
             uuid=instance.uuid)
         mock_get_inst.return_value = instance
 
-        inst_from_get = self.compute_api.get(self.context, instance.uuid)
-
-        inst_map_calls = [mock.call(self.context, instance.uuid),
-                          mock.call(self.context, instance.uuid)]
         if self.cell_type is None:
-            mock_get_inst_map.assert_has_calls(inst_map_calls)
-            self.assertEqual(2, mock_get_inst_map.call_count)
-            mock_get_build_req.assert_called_once_with(self.context,
-                                                       instance.uuid)
-            mock_target_cell.assert_not_called()
-        mock_get_inst.assert_called_once_with(self.context, instance.uuid,
-                                              expected_attrs=[
-                                                  'metadata',
-                                                  'system_metadata',
-                                                  'security_groups',
-                                                  'info_cache'])
-        self.assertEqual(instance, inst_from_get)
+            self.assertRaises(exception.InstanceNotFound,
+                              self.compute_api.get,
+                              self.context, instance.uuid)
+        else:
+            inst_from_get = self.compute_api.get(self.context, instance.uuid)
+
+            mock_get_inst.assert_called_once_with(self.context,
+                                                  instance.uuid,
+                                                  expected_attrs=[
+                                                      'metadata',
+                                                      'system_metadata',
+                                                      'security_groups',
+                                                      'info_cache'])
+            self.assertEqual(instance, inst_from_get)
 
     @mock.patch.object(context, 'target_cell')
     @mock.patch.object(objects.InstanceMapping, 'get_by_instance_uuid')