From a7de4917a0c66a1e1f98e5c7852eeb8b060e69c1 Mon Sep 17 00:00:00 2001 From: melanie witt Date: Thu, 21 Feb 2019 19:17:33 +0000 Subject: [PATCH] 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 --- nova/cmd/manage.py | 6 ++- nova/compute/api.py | 21 +++++++++- nova/objects/virtual_interface.py | 1 + .../functional/db/test_virtual_interface.py | 10 +++++ nova/tests/unit/compute/test_compute_api.py | 39 ++++++++++++++++++- nova/tests/unit/test_nova_manage.py | 16 ++++---- 6 files changed, 81 insertions(+), 12 deletions(-) diff --git a/nova/cmd/manage.py b/nova/cmd/manage.py index 4c18816c5b..8ad8bd1af0 100644 --- a/nova/cmd/manage.py +++ b/nova/cmd/manage.py @@ -1199,6 +1199,7 @@ class CellV2Commands(object): 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 @@ class CellV2Commands(object): # 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 diff --git a/nova/compute/api.py b/nova/compute/api.py index 77012d83c3..95a6789960 100644 --- a/nova/compute/api.py +++ b/nova/compute/api.py @@ -1012,6 +1012,7 @@ class API(base.Base): 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 @@ class API(base.Base): 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 @@ class API(base.Base): 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,7 +2506,7 @@ class API(base.Base): # 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, @@ -2499,6 +2514,8 @@ class API(base.Base): 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. diff --git a/nova/objects/virtual_interface.py b/nova/objects/virtual_interface.py index 96e8759858..b44eeb83ef 100644 --- a/nova/objects/virtual_interface.py +++ b/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. diff --git a/nova/tests/functional/db/test_virtual_interface.py b/nova/tests/functional/db/test_virtual_interface.py index 9cfcc83141..6b9172e4a8 100644 --- a/nova/tests/functional/db/test_virtual_interface.py +++ b/nova/tests/functional/db/test_virtual_interface.py @@ -323,6 +323,16 @@ class VirtualInterfaceListMigrationTestCase( 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) diff --git a/nova/tests/unit/compute/test_compute_api.py b/nova/tests/unit/compute/test_compute_api.py index 023801fd72..2ab7f405e2 100644 --- a/nova/tests/unit/compute/test_compute_api.py +++ b/nova/tests/unit/compute/test_compute_api.py @@ -4878,6 +4878,8 @@ class _ComputeAPIUnitTestMixIn(object): 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 @@ class _ComputeAPIUnitTestMixIn(object): 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 @@ class _ComputeAPIUnitTestMixIn(object): 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 @@ class _ComputeAPIUnitTestMixIn(object): 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 @@ class _ComputeAPIUnitTestMixIn(object): '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 @@ class _ComputeAPIUnitTestMixIn(object): '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 @@ class _ComputeAPIUnitTestMixIn(object): 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) diff --git a/nova/tests/unit/test_nova_manage.py b/nova/tests/unit/test_nova_manage.py index 3e37b7464f..3819b6b6c7 100644 --- a/nova/tests/unit/test_nova_manage.py +++ b/nova/tests/unit/test_nova_manage.py @@ -1198,7 +1198,7 @@ class CellV2CommandsTestCase(test.NoDBTestCase): 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) @@ -1206,6 +1206,8 @@ class CellV2CommandsTestCase(test.NoDBTestCase): 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 @@ class CellV2CommandsTestCase(test.NoDBTestCase): 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 @@ class CellV2CommandsTestCase(test.NoDBTestCase): 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 @@ class CellV2CommandsTestCase(test.NoDBTestCase): 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 @@ class CellV2CommandsTestCase(test.NoDBTestCase): 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 @@ class CellV2CommandsTestCase(test.NoDBTestCase): 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)