Browse Source

Merge "Make _revert_allocation nested allocation aware"

tags/20.0.0.0rc1
Zuul 1 month ago
parent
commit
42cd4337e4
2 changed files with 27 additions and 51 deletions
  1. 6
    20
      nova/compute/manager.py
  2. 21
    31
      nova/tests/unit/compute/test_compute_mgr.py

+ 6
- 20
nova/compute/manager.py View File

@@ -4359,28 +4359,14 @@ class ComputeManager(manager.Manager):
4359 4359
                       migration.uuid, migration.source_node, instance=instance)
4360 4360
             return False
4361 4361
 
4362
-        if len(orig_alloc) > 1:
4363
-            # NOTE(danms): This may change later if we have other allocations
4364
-            # against other providers that need to be held by the migration
4365
-            # as well. Perhaps something like shared storage resources that
4366
-            # will actually be duplicated during a resize type operation.
4367
-            LOG.error('Migration %(mig)s has allocations against '
4368
-                      'more than one provider %(rps)s. This should not be '
4369
-                      'possible, but reverting it anyway.',
4370
-                      {'mig': migration.uuid,
4371
-                       'rps': ','.join(orig_alloc.keys())},
4372
-                      instance=instance)
4373
-
4374
-        # We only have a claim against one provider, it is the source node
4375
-        cn_uuid = list(orig_alloc.keys())[0]
4376
-
4377
-        # FIXME(danms): This method is flawed in that it asssumes allocations
4378
-        # against only one provider. So, this may overwite allocations against
4379
-        # a shared provider, if we had one.
4380
-        LOG.info('Swapping old allocation on %(node)s held by migration '
4362
+        LOG.info('Swapping old allocation on %(rp_uuids)s held by migration '
4381 4363
                  '%(mig)s for instance',
4382
-                 {'node': cn_uuid, 'mig': migration.uuid},
4364
+                 {'rp_uuids': orig_alloc.keys(), 'mig': migration.uuid},
4383 4365
                  instance=instance)
4366
+        # FIXME(gibi): This method is flawed in that it assumes every
4367
+        # allocation held by the migration uuid are against the destination
4368
+        # compute RP tree. So it might overwrite allocation against a
4369
+        # shared provider if we had one.
4384 4370
         # TODO(cdent): Should we be doing anything with return values here?
4385 4371
         self.reportclient.move_allocations(context, migration.uuid,
4386 4372
                                            instance.uuid)

+ 21
- 31
nova/tests/unit/compute/test_compute_mgr.py View File

@@ -7744,13 +7744,16 @@ class ComputeManagerMigrationTestCase(test.NoDBTestCase,
7744 7744
         mock_report.delete_allocation_for_instance.assert_called_once_with(
7745 7745
             self.context, self.migration.uuid, consumer_type='migration')
7746 7746
 
7747
-    def test_revert_allocation(self):
7747
+    def test_revert_allocation_allocation_exists(self):
7748 7748
         """New-style migration-based allocation revert."""
7749 7749
 
7750
+        @mock.patch('nova.compute.manager.LOG.info')
7750 7751
         @mock.patch.object(self.compute, 'reportclient')
7751
-        def doit(mock_report):
7752
-            cu = uuids.node
7753
-            a = {cu: {'resources': {'DISK_GB': 1}}}
7752
+        def doit(mock_report, mock_info):
7753
+            a = {
7754
+                uuids.node: {'resources': {'DISK_GB': 1}},
7755
+                uuids.child_rp: {'resources': {'CUSTOM_FOO': 1}}
7756
+            }
7754 7757
             mock_report.get_allocations_for_consumer.return_value = a
7755 7758
             self.migration.uuid = uuids.migration
7756 7759
 
@@ -7760,14 +7763,20 @@ class ComputeManagerMigrationTestCase(test.NoDBTestCase,
7760 7763
             self.assertTrue(r)
7761 7764
             mock_report.move_allocations.assert_called_once_with(
7762 7765
                 mock.sentinel.ctx, self.migration.uuid, self.instance.uuid)
7766
+            mock_info.assert_called_once_with(
7767
+                'Swapping old allocation on %(rp_uuids)s held by migration '
7768
+                '%(mig)s for instance',
7769
+                {'rp_uuids': a.keys(), 'mig': self.migration.uuid},
7770
+                instance=self.instance)
7763 7771
 
7764 7772
         doit()
7765 7773
 
7766
-    def test_revert_allocation_old_style(self):
7774
+    def test_revert_allocation_allocation_not_exist(self):
7767 7775
         """Test that we don't delete allocs for migration if none found."""
7768 7776
 
7777
+        @mock.patch('nova.compute.manager.LOG.error')
7769 7778
         @mock.patch.object(self.compute, 'reportclient')
7770
-        def doit(mock_report):
7779
+        def doit(mock_report, mock_error):
7771 7780
             mock_report.get_allocations_for_consumer.return_value = {}
7772 7781
             self.migration.uuid = uuids.migration
7773 7782
 
@@ -7776,31 +7785,12 @@ class ComputeManagerMigrationTestCase(test.NoDBTestCase,
7776 7785
 
7777 7786
             self.assertFalse(r)
7778 7787
             self.assertFalse(mock_report.move_allocations.called)
7779
-
7780
-        doit()
7781
-
7782
-    def test_revert_allocation_new_style_unpossible(self):
7783
-        """Test for the should-not-be-possible case of multiple old allocs.
7784
-
7785
-        This should not be a thing that can happen, but just verify that
7786
-        we fall through and guess at one of them. There's not really much else
7787
-        we can do.
7788
-        """
7789
-
7790
-        @mock.patch.object(self.compute, 'reportclient')
7791
-        def doit(mock_report):
7792
-            a = {
7793
-                uuids.node: {'resources': {'DISK_GB': 1}},
7794
-                uuids.edon: {'resources': {'DISK_GB': 1}},
7795
-            }
7796
-            mock_report.get_allocations_for_consumer.return_value = a
7797
-            self.migration.uuid = uuids.migration
7798
-
7799
-            r = self.compute._revert_allocation(mock.sentinel.ctx,
7800
-                                                self.instance, self.migration)
7801
-
7802
-            self.assertTrue(r)
7803
-            self.assertTrue(mock_report.move_allocations.called)
7788
+            mock_error.assert_called_once_with(
7789
+                'Did not find resource allocations for migration '
7790
+                '%s on source node %s. Unable to revert source node '
7791
+                'allocations back to the instance.',
7792
+                self.migration.uuid, self.migration.source_node,
7793
+                instance=self.instance)
7804 7794
 
7805 7795
         doit()
7806 7796
 

Loading…
Cancel
Save