diff --git a/nova/compute/manager.py b/nova/compute/manager.py index 35fd5846232a..c3041c77b29b 100644 --- a/nova/compute/manager.py +++ b/nova/compute/manager.py @@ -4359,28 +4359,14 @@ class ComputeManager(manager.Manager): migration.uuid, migration.source_node, instance=instance) return False - if len(orig_alloc) > 1: - # NOTE(danms): This may change later if we have other allocations - # against other providers that need to be held by the migration - # as well. Perhaps something like shared storage resources that - # will actually be duplicated during a resize type operation. - LOG.error('Migration %(mig)s has allocations against ' - 'more than one provider %(rps)s. This should not be ' - 'possible, but reverting it anyway.', - {'mig': migration.uuid, - 'rps': ','.join(orig_alloc.keys())}, - instance=instance) - - # We only have a claim against one provider, it is the source node - cn_uuid = list(orig_alloc.keys())[0] - - # FIXME(danms): This method is flawed in that it asssumes allocations - # against only one provider. So, this may overwite allocations against - # a shared provider, if we had one. - LOG.info('Swapping old allocation on %(node)s held by migration ' + LOG.info('Swapping old allocation on %(rp_uuids)s held by migration ' '%(mig)s for instance', - {'node': cn_uuid, 'mig': migration.uuid}, + {'rp_uuids': orig_alloc.keys(), 'mig': migration.uuid}, instance=instance) + # FIXME(gibi): This method is flawed in that it assumes every + # allocation held by the migration uuid are against the destination + # compute RP tree. So it might overwrite allocation against a + # shared provider if we had one. # TODO(cdent): Should we be doing anything with return values here? self.reportclient.move_allocations(context, migration.uuid, instance.uuid) diff --git a/nova/tests/unit/compute/test_compute_mgr.py b/nova/tests/unit/compute/test_compute_mgr.py index 755096981058..8fbcd2ea37cf 100644 --- a/nova/tests/unit/compute/test_compute_mgr.py +++ b/nova/tests/unit/compute/test_compute_mgr.py @@ -7744,13 +7744,16 @@ class ComputeManagerMigrationTestCase(test.NoDBTestCase, mock_report.delete_allocation_for_instance.assert_called_once_with( self.context, self.migration.uuid, consumer_type='migration') - def test_revert_allocation(self): + def test_revert_allocation_allocation_exists(self): """New-style migration-based allocation revert.""" + @mock.patch('nova.compute.manager.LOG.info') @mock.patch.object(self.compute, 'reportclient') - def doit(mock_report): - cu = uuids.node - a = {cu: {'resources': {'DISK_GB': 1}}} + def doit(mock_report, mock_info): + a = { + uuids.node: {'resources': {'DISK_GB': 1}}, + uuids.child_rp: {'resources': {'CUSTOM_FOO': 1}} + } mock_report.get_allocations_for_consumer.return_value = a self.migration.uuid = uuids.migration @@ -7760,14 +7763,20 @@ class ComputeManagerMigrationTestCase(test.NoDBTestCase, self.assertTrue(r) mock_report.move_allocations.assert_called_once_with( mock.sentinel.ctx, self.migration.uuid, self.instance.uuid) + mock_info.assert_called_once_with( + 'Swapping old allocation on %(rp_uuids)s held by migration ' + '%(mig)s for instance', + {'rp_uuids': a.keys(), 'mig': self.migration.uuid}, + instance=self.instance) doit() - def test_revert_allocation_old_style(self): + def test_revert_allocation_allocation_not_exist(self): """Test that we don't delete allocs for migration if none found.""" + @mock.patch('nova.compute.manager.LOG.error') @mock.patch.object(self.compute, 'reportclient') - def doit(mock_report): + def doit(mock_report, mock_error): mock_report.get_allocations_for_consumer.return_value = {} self.migration.uuid = uuids.migration @@ -7776,31 +7785,12 @@ class ComputeManagerMigrationTestCase(test.NoDBTestCase, self.assertFalse(r) self.assertFalse(mock_report.move_allocations.called) - - doit() - - def test_revert_allocation_new_style_unpossible(self): - """Test for the should-not-be-possible case of multiple old allocs. - - This should not be a thing that can happen, but just verify that - we fall through and guess at one of them. There's not really much else - we can do. - """ - - @mock.patch.object(self.compute, 'reportclient') - def doit(mock_report): - a = { - uuids.node: {'resources': {'DISK_GB': 1}}, - uuids.edon: {'resources': {'DISK_GB': 1}}, - } - mock_report.get_allocations_for_consumer.return_value = a - self.migration.uuid = uuids.migration - - r = self.compute._revert_allocation(mock.sentinel.ctx, - self.instance, self.migration) - - self.assertTrue(r) - self.assertTrue(mock_report.move_allocations.called) + mock_error.assert_called_once_with( + 'Did not find resource allocations for migration ' + '%s on source node %s. Unable to revert source node ' + 'allocations back to the instance.', + self.migration.uuid, self.migration.source_node, + instance=self.instance) doit()