Merge "Clean PCI claim of failed resize in periodic task"

nova/compute/resource_tracker.py

@@ -1068,6 +1068,15 @@ class ResourceTracker(object):
         self._pair_instances_to_migrations(migrations, instance_by_uuid)
         self._update_usage_from_migrations(context, migrations, nodename)
 
+        # Heal any move claim that leaked on this node as the destination of a
+        # failed move before https://bugs.launchpad.net/nova/+bug/2166786 is
+        # fixed preventing such a leak. This has to run after
+        # _update_usage_from_migrations() populated self.tracked_migrations so
+        # we can tell the leaked claims apart from the legitimately tracked,
+        # in-progress ones.
+        self._release_leaked_move_claims(
+            context, migrations, instance_by_uuid, nodename)
+
         # A new compute node means there won't be a resource provider yet since
         # that would be created via the _update() call below, and if there is
         # no resource provider then there are no allocations against it.
@@ -1636,6 +1645,70 @@ class ResourceTracker(object):
                             instance_uuid=instance.uuid)
                 continue
 
+    def _release_leaked_move_claims(
+        self, context, migrations, instance_by_uuid, nodename,
+    ):
+        """Release move claims leaked on this node as a migration destination.
+
+        A cold migration (or resize) that fails on the source host after the
+        destination already claimed the resources in prep_resize (e.g. PCI
+        devices) leaks those claims on the destination if the destination is
+        never asked to drop them. Such a migration ends up in 'error' state
+        while the instance is reverted back to and kept running on the
+        source host, so nothing releases the claim on the destination.
+
+        New leaks are prevented by asking the destination to drop the claim
+        when the move fails, but pre-existing leaks (from before that fix was
+        deployed) can only be healed here, from the periodic. Detect the leaked
+        claims and release them. See bug https://launchpad.net/bugs/2166786
+        """
+        for migration in migrations:
+            # We only care about claims leaked on this node as the destination
+            # of the move.
+            if not (migration.dest_compute == self.host and
+                    migration.dest_node == nodename):
+                continue
+
+            # An in-progress migration is either accounted for as an incoming
+            # migration (and is present in tracked_migrations) or is still
+            # legitimately holding the claim, so only consider failed ones.
+            if migration.status != 'error':
+                continue
+
+            # If the instance is actually here, or the migration is being
+            # tracked as an in-progress incoming migration, then the claim is
+            # not leaked and is handled by the normal accounting of
+            # tracked_migrations.
+            if (migration.instance_uuid in instance_by_uuid or
+                    migration.instance_uuid in self.tracked_migrations):
+                continue
+
+            # Only PCI devices are claimed on the destination in a way that is
+            # not recomputed from scratch by the periodic (they are persisted
+            # in the DB), so a leak is only observable there. If nothing is
+            # claimed for this instance on this node then there is nothing to
+            # heal.
+            uuid = migration.instance_uuid
+            if not self.pci_tracker or (
+                    uuid not in self.pci_tracker.allocations and
+                    uuid not in self.pci_tracker.claims):
+                continue
+
+            instance = migration.instance
+            LOG.warning(
+                "Releasing a move claim leaked on this host as the "
+                "destination of the failed migration %s. This can happen if "
+                "the migration failed on the source host before the fix for "
+                "bug 2166786 was deployed.",
+                migration.uuid, instance=instance)
+            self.pci_tracker.free_instance(context, instance)
+            # Mark the migration as 'failed' so that it is no longer returned
+            # by MigrationList.get_in_progress_and_error() and therefore this
+            # leak is reconciled exactly once instead of being re-examined on
+            # every periodic run.
+            migration.status = 'failed'
+            migration.save()
+
     def _update_usage_from_instance(self, context, instance, nodename,
             is_removed=False):
         """Update usage for a single instance."""

nova/tests/functional/regressions/test_bug_2166786.py

@@ -42,11 +42,6 @@ class TestPCIColdMigrationBase(base._PCIServersTestBase):
         },
     )]
 
-
-class TestPCIColdMigrationFailureRevert(TestPCIColdMigrationBase):
-    """Regression test for bug https://bugs.launchpad.net/nova/+bug/2166786
-    """
-
     def assert_pci_pool_free_count(self, hostname, free):
         node = objects.ComputeNode.get_by_nodename(self.ctxt, hostname)
         self.assertEqual(
@@ -56,6 +51,11 @@ class TestPCIColdMigrationFailureRevert(TestPCIColdMigrationBase):
         self.assertPCIDeviceCounts(hostname, total=total, free=free)
         self.assert_pci_pool_free_count(hostname, free=free)
 
+
+class TestPCIColdMigrationFailureRevert(TestPCIColdMigrationBase):
+    """Regression test for bug https://bugs.launchpad.net/nova/+bug/2166786
+    """
+
     def test_cold_migrate_server_with_PF(self):
         pci_info = fakelibvirt.HostPCIDevicesInfo(num_pfs=2, num_vfs=0)
         comp0 = self.start_compute(
@@ -97,9 +97,10 @@ class TestPCIColdMigrationFailureRevert(TestPCIColdMigrationBase):
         # the PCI allocation on the destination compute is not cleaned.
         self.assert_pci_inventory(comp1, total=2, free=1)
 
+        # The periodic now detects and cleans such leaks
         self._run_periodics()
         self.assert_pci_inventory(comp0, total=2, free=1)
-        self.assert_pci_inventory(comp1, total=2, free=1)
+        self.assert_pci_inventory(comp1, total=2, free=2)
 
         # NOTE(gibi): This is also the same bug, the migration context is not
         # cleaned up on the instance after the migration failed and reverted.
@@ -108,12 +109,74 @@ class TestPCIColdMigrationFailureRevert(TestPCIColdMigrationBase):
 
         self._delete_server(server)
         self.assert_pci_inventory(comp0, total=2, free=2)
-        # NOTE(gibi): this is also a bit problematic as even
-        # after the VM is deleted the PCI device is still
-        # allocated on the destination compute. We need
-        # and extra periodic run to happen to clean that up.
+        self.assert_pci_inventory(comp1, total=2, free=2)
+
+
+class TestPCIColdMigrationLeakHealedByPeriodic(TestPCIColdMigrationBase):
+    """Regression test for bug https://bugs.launchpad.net/nova/+bug/2166786
+
+    This simulates a leak that happened before the fix that drops the move
+    claim on the destination was deployed, and asserts that the periodic
+    update_available_resource job on the destination is able to heal it.
+    """
+
+    def test_cold_migrate_server_with_PF(self):
+        pci_info = fakelibvirt.HostPCIDevicesInfo(num_pfs=2, num_vfs=0)
+        comp0 = self.start_compute(
+            hostname='test_compute0',
+            pci_info=pci_info)
+        comp1 = self.start_compute(
+            hostname='test_compute1',
+            pci_info=pci_info)
+
+        self.assert_pci_inventory(comp0, total=2, free=2)
+        self.assert_pci_inventory(comp1, total=2, free=2)
+
+        # create a server
+        extra_spec = {"pci_passthrough:alias": "%s:1" % self.PFS_ALIAS_NAME}
+        flavor_id = self._create_flavor(extra_spec=extra_spec)
+        server = self._create_server(
+            flavor_id=flavor_id, networks='none', host='test_compute0')
+
+        self.assert_pci_inventory(comp0, total=2, free=1)
+        self.assert_pci_inventory(comp1, total=2, free=2)
+
+        with mock.patch(
+            'nova.virt.libvirt.driver.LibvirtDriver'
+            '.migrate_disk_and_power_off',
+            side_effect=exception.InstanceFaultRollback(
+                exception.ResizeError(reason="simulated ssh failure"))
+        ):
+            self._migrate_server(server, expected_state="ACTIVE")
+            self._wait_for_migration_status(server, ['error'])
+            self._wait_for_state_change(server, 'ACTIVE')
+            self.notifier.wait_for_versioned_notifications('compute.exception')
+
+        self.assert_pci_inventory(comp0, total=2, free=1)
+        # The migration failed and reverted but, the PCI device claimed on
+        # the destination during prep_resize is leaked.
         self.assert_pci_inventory(comp1, total=2, free=1)
+
+        # The migration is still in error state and holds the leaked claim.
+        migration = self._wait_for_migration_status(server, ['error'])
+
+        # The periodic on the destination detects the leaked move claim of the
+        # failed migration and releases it.
         self._run_periodics()
+        self.assert_pci_inventory(comp0, total=2, free=1)
+        self.assert_pci_inventory(comp1, total=2, free=2)
+
+        # The migration is marked as failed so that it is reconciled only once
+        # and no longer processed by later periodic runs.
+        migration = objects.Migration.get_by_id(self.ctxt, migration['id'])
+        self.assertEqual('failed', migration.status)
+
+        # Running it again is a no-op, the leak stays healed.
+        self._run_periodics()
+        self.assert_pci_inventory(comp0, total=2, free=1)
+        self.assert_pci_inventory(comp1, total=2, free=2)
+
+        self._delete_server(server)
         self.assert_pci_inventory(comp0, total=2, free=2)
         self.assert_pci_inventory(comp1, total=2, free=2)
 
@@ -207,9 +270,10 @@ class TestPCIColdMigrationFailureRevertPciInPlacement(
             traits={"0000:81:00.0": []},
             usages={"0000:81:00.0": {'CUSTOM_PCI_8086_1528': 0}})
 
+        # The periodic now detects and cleans such leaks
         self._run_periodics()
         self.assertPCIDeviceCounts(comp0, total=1, free=0)
-        self.assertPCIDeviceCounts(comp1, total=1, free=0)
+        self.assertPCIDeviceCounts(comp1, total=1, free=1)
         self.assert_placement_pci_view(
             comp0,
             inventories={"0000:81:00.0": {'CUSTOM_PCI_8086_1528': 1}},
@@ -225,25 +289,6 @@ class TestPCIColdMigrationFailureRevertPciInPlacement(
 
         self._delete_server(server)
         self.assertPCIDeviceCounts(comp0, total=1, free=1)
-        # NOTE(gibi): this is also a bit problematic as even
-        # after the VM is deleted the PCI device is still
-        # allocated on the destination compute. We need
-        # and extra periodic run to happen to clean that up.
-        self.assertPCIDeviceCounts(comp1, total=1, free=0)
-
-        self.assert_placement_pci_view(
-            comp0,
-            inventories={"0000:81:00.0": {'CUSTOM_PCI_8086_1528': 1}},
-            traits={"0000:81:00.0": []},
-            usages={"0000:81:00.0": {'CUSTOM_PCI_8086_1528': 0}})
-        self.assert_placement_pci_view(
-            comp1,
-            inventories={"0000:81:00.0": {'CUSTOM_PCI_8086_1528': 1}},
-            traits={"0000:81:00.0": []},
-            usages={"0000:81:00.0": {'CUSTOM_PCI_8086_1528': 0}})
-
-        self._run_periodics()
-        self.assertPCIDeviceCounts(comp0, total=1, free=1)
         self.assertPCIDeviceCounts(comp1, total=1, free=1)
 
         self.assert_placement_pci_view(

nova/tests/unit/compute/test_resource_tracker.py

@@ -4729,3 +4729,192 @@ class TestCleanComputeNodeCache(BaseTestCase):
 
         mock_remove.assert_called_once_with(invalid_nodename)
         mock_invalidate.assert_called_once_with(invalid_nodename)
+
+
+class TestReleaseLeakedMoveClaims(BaseTestCase):
+    """Tests for ResourceTracker._release_leaked_move_claims().
+
+    See bug https://bugs.launchpad.net/nova/+bug/2166786
+    """
+
+    def setUp(self):
+        super().setUp()
+        self._setup_rt()
+        # By default the RT tracks nothing as an in-progress incoming
+        # migration.
+        self.rt.tracked_migrations = {}
+        # Set up a PCI tracker that reports the instance as having leaked
+        # allocations by default. Individual tests override this.
+        self.pci_tracker = mock.Mock(spec_set=[
+            'allocations', 'claims', 'free_instance'])
+        self.pci_tracker.allocations = {uuids.instance: mock.sentinel.devs}
+        self.pci_tracker.claims = {}
+        self.rt.pci_tracker = self.pci_tracker
+
+    def _migration(self, **kwargs):
+        instance = objects.Instance(uuid=uuids.instance)
+        values = dict(
+            uuid=uuids.migration,
+            instance_uuid=uuids.instance,
+            instance=instance,
+            source_compute='other-host',
+            source_node='other-node',
+            dest_compute=_HOSTNAME,
+            dest_node=_NODENAME,
+            status='error',
+        )
+        values.update(kwargs)
+        return objects.Migration(context=mock.sentinel.ctx, **values)
+
+    def _release(self, migrations, instance_by_uuid=None):
+        self.rt._release_leaked_move_claims(
+            mock.sentinel.ctx, migrations,
+            instance_by_uuid if instance_by_uuid is not None else {},
+            _NODENAME)
+
+    @mock.patch('nova.objects.migration.Migration.save')
+    def test_leaked_allocation_released(self, mock_save):
+        migration = self._migration()
+
+        self._release([migration])
+
+        self.pci_tracker.free_instance.assert_called_once_with(
+            mock.sentinel.ctx, migration.instance)
+
+        self.assertEqual('failed', migration.status)
+        mock_save.assert_called_once_with()
+        self.assertIn(
+            'Releasing a move claim leaked on this host',
+            self.stdlog.logger.output)
+
+        # The migration is marked as 'failed' so it is not re-examined on the
+        # next periodic run.
+        self.pci_tracker.reset_mock()
+        mock_save.reset_mock()
+
+        self._release([migration])
+
+        self.pci_tracker.free_instance.assert_not_called()
+        mock_save.assert_not_called()
+
+    @mock.patch('nova.objects.migration.Migration.save')
+    def test_leaked_claim_released(self, mock_save):
+        # The leak may be recorded in pci_tracker.claims rather than
+        # allocations.
+        self.pci_tracker.allocations = {}
+        self.pci_tracker.claims = {uuids.instance: mock.sentinel.devs}
+        migration = self._migration()
+
+        self._release([migration])
+
+        self.pci_tracker.free_instance.assert_called_once_with(
+            mock.sentinel.ctx, migration.instance)
+        self.assertEqual('failed', migration.status)
+        mock_save.assert_called_once_with()
+
+    @mock.patch('nova.objects.migration.Migration.save')
+    def test_migration_to_other_dest_ignored(self, mock_save):
+        # A migration where this node is not the destination is left alone.
+        migration = self._migration(
+            dest_compute='other-host', dest_node='other-node')
+
+        self._release([migration])
+
+        self.pci_tracker.free_instance.assert_not_called()
+        mock_save.assert_not_called()
+        self.assertEqual('error', migration.status)
+
+    @mock.patch('nova.objects.migration.Migration.save')
+    def test_migration_to_other_node_on_same_host_ignored(self, mock_save):
+        # Same host but a different node is not our destination.
+        migration = self._migration(dest_node='other-node')
+
+        self._release([migration])
+
+        self.pci_tracker.free_instance.assert_not_called()
+        mock_save.assert_not_called()
+
+    @mock.patch('nova.objects.migration.Migration.save')
+    def test_non_error_migration_ignored(self, mock_save):
+        # In-progress (or otherwise non-error) migrations are handled by the
+        # normal tracked_migrations accounting and must not be touched here.
+        for status in ('pre-migrating', 'migrating', 'finished', 'confirmed',
+                       'reverted', 'failed', 'done'):
+            self.pci_tracker.free_instance.reset_mock()
+            migration = self._migration(status=status)
+
+            self._release([migration])
+
+            self.pci_tracker.free_instance.assert_not_called()
+            mock_save.assert_not_called()
+
+    @mock.patch('nova.objects.migration.Migration.save')
+    def test_instance_present_on_this_node_ignored(self, mock_save):
+        # If the instance is actually running here the claim is legitimate.
+        migration = self._migration()
+
+        self._release(
+            [migration],
+            instance_by_uuid={uuids.instance: migration.instance})
+
+        self.pci_tracker.free_instance.assert_not_called()
+        mock_save.assert_not_called()
+        self.assertEqual('error', migration.status)
+
+    @mock.patch('nova.objects.migration.Migration.save')
+    def test_tracked_migration_ignored(self, mock_save):
+        # A migration accounted for as an in-progress incoming migration is
+        # not leaked.
+        migration = self._migration()
+        self.rt.tracked_migrations = {uuids.instance: migration}
+
+        self._release([migration])
+
+        self.pci_tracker.free_instance.assert_not_called()
+        mock_save.assert_not_called()
+
+    @mock.patch('nova.objects.migration.Migration.save')
+    def test_no_pci_tracker_ignored(self, mock_save):
+        self.rt.pci_tracker = None
+        migration = self._migration()
+
+        # Should not raise even though there is no pci_tracker to consult.
+        self._release([migration])
+
+        mock_save.assert_not_called()
+        self.assertEqual('error', migration.status)
+
+    @mock.patch('nova.objects.migration.Migration.save')
+    def test_nothing_claimed_for_instance_ignored(self, mock_save):
+        # The instance has no PCI allocation or claim on this node, so there
+        # is nothing leaked to heal.
+        self.pci_tracker.allocations = {}
+        self.pci_tracker.claims = {}
+        migration = self._migration()
+
+        self._release([migration])
+
+        self.pci_tracker.free_instance.assert_not_called()
+        mock_save.assert_not_called()
+        self.assertEqual('error', migration.status)
+
+    @mock.patch('nova.objects.migration.Migration.save')
+    def test_only_leaked_migration_released(self, mock_save):
+        # A mix of migrations: only the genuinely leaked one is healed.
+        leaked = self._migration()
+        # A migration for an instance that is still running here.
+        present_uuid = uuids.instance2
+        present = self._migration(
+            uuid=uuids.migration2, instance_uuid=present_uuid)
+        present.instance = objects.Instance(uuid=present_uuid)
+        self.pci_tracker.allocations[present_uuid] = mock.sentinel.devs2
+
+        self._release(
+            [leaked, present],
+            instance_by_uuid={present_uuid: present.instance})
+
+        self.pci_tracker.free_instance.assert_called_once_with(
+            mock.sentinel.ctx, leaked.instance)
+        self.assertEqual('failed', leaked.status)
+        self.assertEqual('error', present.status)
+        mock_save.assert_called_once_with()