Populate InstanceMapping.user_id during migrations and schedules

The InstanceMapping user_id field is a new, non-nullable field
representing the user_id for the instance.

When new instance create requests come in, we create the instance
mapping. We will set user_id here before creating the record.

Some virtual interface online data migration and map_instances routine
create InstanceMapping records and since the user_id field did not
previously exist, they were not setting it. We will populate user_id in
these cases.

Finally, whenever an API does a compute_api.get(), we can
opportunistically set and save user_id on the instance mapping if it is
not set.

Part of blueprint count-quota-usage-from-placement

Change-Id: Ic4bb7b49b90a3d6d7ce6c6c62d87836f96309f06

Author: melwitt

nova/cmd/manage.py

@@ -1199,6 +1199,7 @@ def _get_and_map_instances(self, ctxt, cell_mapping, limit, marker):
                 mapping.instance_uuid = instance.uuid
                 mapping.cell_mapping = cell_mapping
                 mapping.project_id = instance.project_id
+                mapping.user_id = instance.user_id
                 mapping.create()
             except db_exc.DBDuplicateEntry:
                 continue
@@ -1293,8 +1294,11 @@ def map_instances(self, cell_uuid, max_count=None, reset_marker=None):
             # Don't judge me. There's already an InstanceMapping with this UUID
             # so the marker needs to be non destructively modified.
             next_marker = next_marker.replace('-', ' ')
+            # This is just the marker record, so set user_id to the special
+            # marker name as well.
             objects.InstanceMapping(ctxt, instance_uuid=next_marker,
-                    project_id=marker_project_id).create()
+                    project_id=marker_project_id,
+                    user_id=marker_project_id).create()
             return 1
         return 0
 

nova/compute/api.py

@@ -1012,6 +1012,7 @@ def _provision_instances(self, context, instance_type, min_count,
                 inst_mapping = objects.InstanceMapping(context=context)
                 inst_mapping.instance_uuid = instance_uuid
                 inst_mapping.project_id = context.project_id
+                inst_mapping.user_id = context.user_id
                 inst_mapping.cell_mapping = None
                 inst_mapping.create()
 
@@ -2458,6 +2459,18 @@ def _get_instance_map_or_none(self, context, instance_uuid):
             inst_map = None
         return inst_map
 
+    @staticmethod
+    def _save_user_id_in_instance_mapping(mapping, instance):
+        # TODO(melwitt): We take the opportunity to migrate user_id on the
+        # instance mapping if it's not yet been migrated. This can be removed
+        # in a future release, when all migrations are complete.
+        # If the instance came from a RequestSpec because of a down cell, its
+        # user_id could be None and the InstanceMapping.user_id field is
+        # non-nullable. Avoid trying to set/save the user_id in that case.
+        if 'user_id' not in mapping and instance.user_id is not None:
+            mapping.user_id = instance.user_id
+            mapping.save()
+
     def _get_instance_from_cell(self, context, im, expected_attrs,
                                 cell_down_support):
         # NOTE(danms): Even though we're going to scatter/gather to the
@@ -2471,7 +2484,9 @@ def _get_instance_from_cell(self, context, im, expected_attrs,
             expected_attrs=expected_attrs)
         cell_uuid = im.cell_mapping.uuid
         if not nova_context.is_cell_failure_sentinel(result[cell_uuid]):
-            return result[cell_uuid]
+            inst = result[cell_uuid]
+            self._save_user_id_in_instance_mapping(im, inst)
+            return inst
         elif isinstance(result[cell_uuid], exception.InstanceNotFound):
             raise exception.InstanceNotFound(instance_id=uuid)
         elif cell_down_support:
@@ -2491,14 +2506,16 @@ def _get_instance_from_cell(self, context, im, expected_attrs,
                 # and its id.
                 image_ref = (rs.image.id if rs.image and
                              'id' in rs.image else None)
-                return objects.Instance(context=context, power_state=0,
+                inst = objects.Instance(context=context, power_state=0,
                                         uuid=uuid,
                                         project_id=im.project_id,
                                         created_at=im.created_at,
                                         user_id=rs.user_id,
                                         flavor=rs.flavor,
                                         image_ref=image_ref,
                                         availability_zone=rs.availability_zone)
+                self._save_user_id_in_instance_mapping(im, inst)
+                return inst
             except exception.RequestSpecNotFound:
                 # could be that a deleted instance whose request
                 # spec has been archived is being queried.

nova/objects/virtual_interface.py

@@ -301,6 +301,7 @@ def _set_or_delete_marker_for_migrate_instances(context, marker=None):
         instance = objects.Instance(context)
         instance.uuid = FAKE_UUID
         instance.project_id = FAKE_UUID
+        instance.user_id = FAKE_UUID
         instance.create()
         # Thats fake instance, lets destroy it.
         # We need only its row to solve constraint issue.

nova/tests/functional/db/test_virtual_interface.py

@@ -323,6 +323,16 @@ def test_migration_multiple_cells(self):
         self.assertEqual(4, match)
         self.assertEqual(3, done)
 
+        # Verify that the marker instance has project_id/user_id set properly.
+        with context.target_cell(self.context, self.cells[1]) as cctxt:
+            # The marker record is destroyed right after it's created, since
+            # only the presence of the row is needed to satisfy the fkey
+            # constraint.
+            cctxt = cctxt.elevated(read_deleted='yes')
+            marker_instance = objects.Instance.get_by_uuid(cctxt, FAKE_UUID)
+        self.assertEqual(FAKE_UUID, marker_instance.project_id)
+        self.assertEqual(FAKE_UUID, marker_instance.user_id)
+
         # Try again - should fill 3 left instances from cell1
         match, done = virtual_interface.fill_virtual_interface_list(
             self.context, 4)

nova/tests/unit/compute/test_compute_api.py

@@ -4878,6 +4878,8 @@ def do_test(mock_inst_mapping, mock_check_num_inst_quota):
                              inst_mapping_mock.instance_uuid)
             self.assertIsNone(inst_mapping_mock.cell_mapping)
             self.assertEqual(ctxt.project_id, inst_mapping_mock.project_id)
+            # Verify that the instance mapping created has user_id populated.
+            self.assertEqual(ctxt.user_id, inst_mapping_mock.user_id)
         do_test()
 
     @mock.patch.object(objects.service, 'get_minimum_version_all_cells',
@@ -5759,6 +5761,8 @@ def test_get_all_with_cell_down_support_all_tenants(self, mock_get_ims,
                                                  None,
                                                  limit=3)
 
+    @mock.patch('nova.compute.api.API._save_user_id_in_instance_mapping',
+                new=mock.MagicMock())
     @mock.patch.object(objects.Instance, 'get_by_uuid')
     def test_get_instance_from_cell_success(self, mock_get_inst):
         cell_mapping = objects.CellMapping(uuid=uuids.cell1,
@@ -5786,9 +5790,11 @@ def test_get_instance_from_cell_failure(self, mock_get_inst):
             im, [], False)
         self.assertIn('could not be found', six.text_type(exp))
 
+    @mock.patch('nova.compute.api.API._save_user_id_in_instance_mapping')
     @mock.patch.object(objects.RequestSpec, 'get_by_instance_uuid')
     @mock.patch('nova.context.scatter_gather_cells')
-    def test_get_instance_with_cell_down_support(self, mock_sg, mock_rs):
+    def test_get_instance_with_cell_down_support(self, mock_sg, mock_rs,
+                                                 mock_save_uid):
         cell_mapping = objects.CellMapping(uuid=uuids.cell1,
                                            name='1', id=1)
         im1 = objects.InstanceMapping(instance_uuid=uuids.inst1,
@@ -5840,6 +5846,8 @@ def test_get_instance_with_cell_down_support(self, mock_sg, mock_rs):
         self.assertEqual(uuids.inst2, result.uuid)
         self.assertEqual('nova', result.availability_zone)
         self.assertEqual(uuids.image, result.image_ref)
+        # Verify that user_id is populated during a compute_api.get().
+        mock_save_uid.assert_called_once_with(im2, result)
 
         # Same as above, but boot-from-volume where image is not None but the
         # id of the image is not set.
@@ -5903,6 +5911,8 @@ def test_get_instance_not_in_cell(self, mock_get_inst, mock_get_build_req,
                                 'security_groups', 'info_cache'])
         self.assertEqual(instance, inst_from_build_req)
 
+    @mock.patch('nova.compute.api.API._save_user_id_in_instance_mapping',
+                new=mock.MagicMock())
     @mock.patch.object(objects.InstanceMapping, 'get_by_instance_uuid')
     @mock.patch.object(objects.BuildRequest, 'get_by_instance_uuid')
     @mock.patch.object(objects.Instance, 'get_by_uuid')
@@ -5988,11 +5998,34 @@ def test_get_instance_not_in_cell_buildreq_deleted_inst_still_not_in_cell(
                                                       'info_cache'])
             self.assertEqual(instance, inst_from_get)
 
+    @mock.patch('nova.objects.InstanceMapping.save')
+    def test_save_user_id_in_instance_mapping(self, im_save):
+        # Verify user_id is populated if it not set
+        im = objects.InstanceMapping()
+        i = objects.Instance(user_id='fake')
+        self.compute_api._save_user_id_in_instance_mapping(im, i)
+        self.assertEqual(im.user_id, i.user_id)
+        im_save.assert_called_once_with()
+        # Verify user_id is not saved if it is already set
+        im_save.reset_mock()
+        im.user_id = 'fake-other'
+        self.compute_api._save_user_id_in_instance_mapping(im, i)
+        self.assertNotEqual(im.user_id, i.user_id)
+        im_save.assert_not_called()
+        # Verify user_id is not saved if it is None
+        im_save.reset_mock()
+        im = objects.InstanceMapping()
+        i = objects.Instance(user_id=None)
+        self.compute_api._save_user_id_in_instance_mapping(im, i)
+        self.assertNotIn('user_id', im)
+        im_save.assert_not_called()
+
+    @mock.patch('nova.compute.api.API._save_user_id_in_instance_mapping')
     @mock.patch.object(objects.InstanceMapping, 'get_by_instance_uuid')
     @mock.patch.object(objects.BuildRequest, 'get_by_instance_uuid')
     @mock.patch.object(objects.Instance, 'get_by_uuid')
     def test_get_instance_in_cell(self, mock_get_inst, mock_get_build_req,
-            mock_get_inst_map):
+            mock_get_inst_map, mock_save_uid):
         self.useFixture(nova_fixtures.AllServicesCurrent())
         # This just checks that the instance is looked up normally and not
         # synthesized from a BuildRequest object. Verification of pulling the
@@ -6010,6 +6043,8 @@ def test_get_instance_in_cell(self, mock_get_inst, mock_get_build_req,
         if self.cell_type is None:
             mock_get_inst_map.assert_called_once_with(self.context,
                                                       instance.uuid)
+            # Verify that user_id is populated during a compute_api.get().
+            mock_save_uid.assert_called_once_with(inst_map, instance)
         else:
             self.assertFalse(mock_get_inst_map.called)
         self.assertEqual(instance, returned_inst)

nova/tests/unit/test_nova_manage.py

@@ -1198,14 +1198,16 @@ def test_map_instances(self, mock_target_cell):
             uuid = uuidutils.generate_uuid()
             instance_uuids.append(uuid)
             objects.Instance(ctxt, project_id=ctxt.project_id,
-                             uuid=uuid).create()
+                             user_id=ctxt.user_id, uuid=uuid).create()
 
         self.commands.map_instances(cell_uuid)
 
         for uuid in instance_uuids:
             inst_mapping = objects.InstanceMapping.get_by_instance_uuid(ctxt,
                     uuid)
             self.assertEqual(ctxt.project_id, inst_mapping.project_id)
+            # Verify that map_instances populates user_id.
+            self.assertEqual(ctxt.user_id, inst_mapping.user_id)
             self.assertEqual(cell_mapping.uuid, inst_mapping.cell_mapping.uuid)
         mock_target_cell.assert_called_once_with(
             test.MatchType(context.RequestContext),
@@ -1225,10 +1227,10 @@ def test_map_instances_duplicates(self, mock_target_cell):
             uuid = uuidutils.generate_uuid()
             instance_uuids.append(uuid)
             objects.Instance(ctxt, project_id=ctxt.project_id,
-                             uuid=uuid).create()
+                             user_id=ctxt.user_id, uuid=uuid).create()
 
         objects.InstanceMapping(ctxt, project_id=ctxt.project_id,
-                instance_uuid=instance_uuids[0],
+                user_id=ctxt.user_id, instance_uuid=instance_uuids[0],
                 cell_mapping=cell_mapping).create()
 
         self.commands.map_instances(cell_uuid)
@@ -1260,7 +1262,7 @@ def test_map_instances_two_batches(self, mock_target_cell):
             uuid = uuidutils.generate_uuid()
             instance_uuids.append(uuid)
             objects.Instance(ctxt, project_id=ctxt.project_id,
-                             uuid=uuid).create()
+                             user_id=ctxt.user_id, uuid=uuid).create()
 
         ret = self.commands.map_instances(cell_uuid)
         self.assertEqual(0, ret)
@@ -1292,7 +1294,7 @@ def test_map_instances_max_count(self, mock_target_cell):
             uuid = uuidutils.generate_uuid()
             instance_uuids.append(uuid)
             objects.Instance(ctxt, project_id=ctxt.project_id,
-                             uuid=uuid).create()
+                             user_id=ctxt.user_id, uuid=uuid).create()
 
         ret = self.commands.map_instances(cell_uuid, max_count=3)
         self.assertEqual(1, ret)
@@ -1329,7 +1331,7 @@ def test_map_instances_marker_deleted(self, mock_target_cell):
             uuid = uuidutils.generate_uuid()
             instance_uuids.append(uuid)
             objects.Instance(ctxt, project_id=ctxt.project_id,
-                             uuid=uuid).create()
+                             user_id=ctxt.user_id, uuid=uuid).create()
 
         ret = self.commands.map_instances(cell_uuid, max_count=3)
         self.assertEqual(1, ret)
@@ -1371,7 +1373,7 @@ def test_map_instances_marker_reset(self, mock_target_cell):
             uuid = uuidutils.generate_uuid()
             instance_uuids.append(uuid)
             objects.Instance(ctxt, project_id=ctxt.project_id,
-                             uuid=uuid).create()
+                             user_id=ctxt.user_id, uuid=uuid).create()
 
         # Maps first three instances.
         ret = self.commands.map_instances(cell_uuid, max_count=3)