From 6310e693039cc3b6d2808915a3b4daf7bbffb9a8 Mon Sep 17 00:00:00 2001 From: Matt Riedemann Date: Tue, 21 Aug 2018 15:32:13 -0400 Subject: [PATCH] libvirt: check job status for VIR_DOMAIN_EVENT_SUSPENDED_MIGRATED event Change Ic5cab99944df9e501ba2032eb96911c36304494d added handling for the VIR_DOMAIN_EVENT_SUSPENDED_MIGRATED event during live migration but failed to distinguish between the live migration actually succeeding or failing before queueing the EVENT_LIFECYCLE_MIGRATION_COMPLETED up into the ComputeManager.handle_lifecycle_event method. As a result, failed live migrations will inadvertantly trigger activation of the port bindings on the destination host, which deactivates the source host port bindings, and then _rollback_live_migration will delete those activated dest host port bindings and leave the source host port bindings deactivated. In this change, if we get the VIR_DOMAIN_EVENT_SUSPENDED_MIGRATED event, we attempt to get the job status to determine the course to take and only queue the EVENT_LIFECYCLE_MIGRATION_COMPLETED event, which triggers the dest host port activation, if we can determine the live migration job completed successfully. Otherwise we simply report the guest as paused, the same as before Ic5cab9994. Conflicts: nova/virt/libvirt/host.py NOTE(mriedem): The conflict on the imports is due to not having change I4aeac9b2397bb2f5e130d1e58829a5e549fcb191 in Stein. Change-Id: I6a4252b0c12c41c233299f30ce8294fef21c7b40 Closes-Bug: #1788014 (cherry picked from commit aa87b9c288d316b85079e681e0df24354ec1912c) (cherry picked from commit 27bfd0bc6233c25114504bb363402807752a7ece) --- nova/tests/unit/virt/libvirt/fakelibvirt.py | 1 + nova/tests/unit/virt/libvirt/test_host.py | 54 ++++++++++++++----- .../tests/unit/virt/libvirt/test_migration.py | 8 +++ nova/virt/libvirt/host.py | 28 +++++++--- nova/virt/libvirt/migration.py | 28 ++++++---- 5 files changed, 88 insertions(+), 31 deletions(-) diff --git a/nova/tests/unit/virt/libvirt/fakelibvirt.py b/nova/tests/unit/virt/libvirt/fakelibvirt.py index c00e9f79f914..62444ed8f2f8 100644 --- a/nova/tests/unit/virt/libvirt/fakelibvirt.py +++ b/nova/tests/unit/virt/libvirt/fakelibvirt.py @@ -73,6 +73,7 @@ VIR_DOMAIN_EVENT_STOPPED = 5 VIR_DOMAIN_EVENT_SHUTDOWN = 6 VIR_DOMAIN_EVENT_PMSUSPENDED = 7 +VIR_DOMAIN_EVENT_SUSPENDED_MIGRATED = 1 VIR_DOMAIN_EVENT_SUSPENDED_POSTCOPY = 7 VIR_DOMAIN_UNDEFINE_MANAGED_SAVE = 1 diff --git a/nova/tests/unit/virt/libvirt/test_host.py b/nova/tests/unit/virt/libvirt/test_host.py index 9d0e6d23330e..8d0ee1e44057 100644 --- a/nova/tests/unit/virt/libvirt/test_host.py +++ b/nova/tests/unit/virt/libvirt/test_host.py @@ -210,7 +210,8 @@ class HostTestCase(test.NoDBTestCase): self.assertEqual(event.EVENT_LIFECYCLE_POSTCOPY_STARTED, expected_event.transition) - def test_event_lifecycle_callback_suspended_migrated(self): + @mock.patch('nova.virt.libvirt.guest.Guest.get_job_info') + def test_event_lifecycle_callback_suspended_migrated(self, get_job_info): """Tests the suspended lifecycle event with libvirt with migrated""" hostimpl = mock.MagicMock() conn = mock.MagicMock() @@ -220,22 +221,47 @@ class HostTestCase(test.NoDBTestCase): """ dom = fakelibvirt.Domain(conn, fake_dom_xml, running=True) - # See https://libvirt.org/html/libvirt-libvirt-domain.html for values. - VIR_DOMAIN_EVENT_SUSPENDED_MIGRATED = 1 - with mock.patch.object(host.libvirt, - 'VIR_DOMAIN_EVENT_SUSPENDED_MIGRATED', new=1, - create=True): - host.Host._event_lifecycle_callback( - conn, dom, fakelibvirt.VIR_DOMAIN_EVENT_SUSPENDED, - detail=VIR_DOMAIN_EVENT_SUSPENDED_MIGRATED, opaque=hostimpl) + jobinfo = libvirt_guest.JobInfo( + type=fakelibvirt.VIR_DOMAIN_JOB_COMPLETED) + get_job_info.return_value = jobinfo + host.Host._event_lifecycle_callback( + conn, dom, fakelibvirt.VIR_DOMAIN_EVENT_SUSPENDED, + detail=fakelibvirt.VIR_DOMAIN_EVENT_SUSPENDED_MIGRATED, + opaque=hostimpl) + expected_event = hostimpl._queue_event.call_args[0][0] + self.assertEqual(event.EVENT_LIFECYCLE_MIGRATION_COMPLETED, + expected_event.transition) + get_job_info.assert_called_once_with() + + @mock.patch('nova.virt.libvirt.guest.Guest.get_job_info') + @mock.patch('nova.virt.libvirt.migration.find_job_type') + def test_event_lifecycle_callback_suspended_migrated_job_failed( + self, find_job_type, get_job_info): + """Tests the suspended lifecycle event with libvirt with migrated""" + hostimpl = mock.MagicMock() + conn = mock.MagicMock() + fake_dom_xml = """ + + cef19ce0-0ca2-11df-855d-b19fbce37686 + + """ + dom = fakelibvirt.Domain(conn, fake_dom_xml, running=True) + jobinfo = libvirt_guest.JobInfo(type=fakelibvirt.VIR_DOMAIN_JOB_NONE) + get_job_info.return_value = jobinfo + # If the job type is VIR_DOMAIN_JOB_NONE we'll attempt to figure out + # the actual job status, so in this case we mock it to be a failure. + find_job_type.return_value = fakelibvirt.VIR_DOMAIN_JOB_FAILED + host.Host._event_lifecycle_callback( + conn, dom, fakelibvirt.VIR_DOMAIN_EVENT_SUSPENDED, + detail=fakelibvirt.VIR_DOMAIN_EVENT_SUSPENDED_MIGRATED, + opaque=hostimpl) expected_event = hostimpl._queue_event.call_args[0][0] - # FIXME(mriedem): This should be EVENT_LIFECYCLE_MIGRATION_COMPLETED - # once bug 1788014 is fixed and we properly check job status for the - # VIR_DOMAIN_EVENT_SUSPENDED_MIGRATED case. - # self.assertEqual(event.EVENT_LIFECYCLE_MIGRATION_COMPLETED, - # expected_event.transition) self.assertEqual(event.EVENT_LIFECYCLE_PAUSED, expected_event.transition) + get_job_info.assert_called_once_with() + find_job_type.assert_called_once_with( + test.MatchType(libvirt_guest.Guest), instance=None, + logging_ok=False) def test_event_emit_delayed_call_delayed(self): ev = event.LifecycleEvent( diff --git a/nova/tests/unit/virt/libvirt/test_migration.py b/nova/tests/unit/virt/libvirt/test_migration.py index b4d8536b5248..f75698b019ea 100644 --- a/nova/tests/unit/virt/libvirt/test_migration.py +++ b/nova/tests/unit/virt/libvirt/test_migration.py @@ -973,6 +973,14 @@ class MigrationMonitorTestCase(test.NoDBTestCase): self.assertEqual(migration.find_job_type(self.guest, self.instance), fakelibvirt.VIR_DOMAIN_JOB_FAILED) + @mock.patch('nova.virt.libvirt.migration.LOG', + new_callable=mock.NonCallableMock) # asserts not called + @mock.patch('nova.virt.libvirt.guest.Guest.is_active', return_value=True) + def test_live_migration_find_type_no_logging(self, mock_active, _mock_log): + self.assertEqual(fakelibvirt.VIR_DOMAIN_JOB_FAILED, + migration.find_job_type(self.guest, self.instance, + logging_ok=False)) + def test_live_migration_abort_too_long(self): # Elapsed time is over completion timeout self.assertTrue(migration.should_trigger_timeout_action( diff --git a/nova/virt/libvirt/host.py b/nova/virt/libvirt/host.py index ef7c0c2b1418..de6a68e83f09 100644 --- a/nova/virt/libvirt/host.py +++ b/nova/virt/libvirt/host.py @@ -56,6 +56,7 @@ from nova import utils from nova.virt import event as virtevent from nova.virt.libvirt import config as vconfig from nova.virt.libvirt import guest as libvirt_guest +from nova.virt.libvirt import migration as libvirt_migrate libvirt = None @@ -174,12 +175,27 @@ class Host(object): elif event == libvirt.VIR_DOMAIN_EVENT_SUSPENDED: if detail == libvirt.VIR_DOMAIN_EVENT_SUSPENDED_POSTCOPY: transition = virtevent.EVENT_LIFECYCLE_POSTCOPY_STARTED - # FIXME(mriedem): VIR_DOMAIN_EVENT_SUSPENDED_MIGRATED is also sent - # when live migration of the guest fails, so we cannot simply rely - # on the event itself but need to check if the job itself was - # successful. - # elif detail == libvirt.VIR_DOMAIN_EVENT_SUSPENDED_MIGRATED: - # transition = virtevent.EVENT_LIFECYCLE_MIGRATION_COMPLETED + elif detail == libvirt.VIR_DOMAIN_EVENT_SUSPENDED_MIGRATED: + # VIR_DOMAIN_EVENT_SUSPENDED_MIGRATED is also sent when live + # migration of the guest fails, so we cannot simply rely + # on the event itself but need to check if the job itself was + # successful. + # NOTE(mriedem): The job check logic here is copied from + # LibvirtDriver._live_migration_monitor. + guest = libvirt_guest.Guest(dom) + info = guest.get_job_info() + if info.type == libvirt.VIR_DOMAIN_JOB_NONE: + # Either still running, or failed or completed, + # lets untangle the mess. + info.type = libvirt_migrate.find_job_type( + guest, instance=None, logging_ok=False) + + if info.type == libvirt.VIR_DOMAIN_JOB_COMPLETED: + transition = virtevent.EVENT_LIFECYCLE_MIGRATION_COMPLETED + else: + # Failed or some other status we don't know about, so just + # opt to report the guest is paused. + transition = virtevent.EVENT_LIFECYCLE_PAUSED else: transition = virtevent.EVENT_LIFECYCLE_PAUSED elif event == libvirt.VIR_DOMAIN_EVENT_RESUMED: diff --git a/nova/virt/libvirt/migration.py b/nova/virt/libvirt/migration.py index 3ad803abbcd3..36385878c691 100644 --- a/nova/virt/libvirt/migration.py +++ b/nova/virt/libvirt/migration.py @@ -340,11 +340,13 @@ def _update_vif_xml(xml_doc, migrate_data, get_vif_config): return xml_doc -def find_job_type(guest, instance): +def find_job_type(guest, instance, logging_ok=True): """Determine the (likely) current migration job type :param guest: a nova.virt.libvirt.guest.Guest :param instance: a nova.objects.Instance + :param logging_ok: If logging in this method is OK. If called from a + native thread then logging is generally prohibited. Annoyingly when job type == NONE and migration is no longer running, we don't know whether we stopped @@ -354,25 +356,29 @@ def find_job_type(guest, instance): :returns: a libvirt job type constant """ + def _log(func, msg, *args, **kwargs): + if logging_ok: + func(msg, *args, **kwargs) + try: if guest.is_active(): - LOG.debug("VM running on src, migration failed", - instance=instance) + _log(LOG.debug, "VM running on src, migration failed", + instance=instance) return libvirt.VIR_DOMAIN_JOB_FAILED else: - LOG.debug("VM is shutoff, migration finished", - instance=instance) + _log(LOG.debug, "VM is shutoff, migration finished", + instance=instance) return libvirt.VIR_DOMAIN_JOB_COMPLETED except libvirt.libvirtError as ex: - LOG.debug("Error checking domain status %(ex)s", - {"ex": ex}, instance=instance) + _log(LOG.debug, "Error checking domain status %(ex)s", {"ex": ex}, + instance=instance) if ex.get_error_code() == libvirt.VIR_ERR_NO_DOMAIN: - LOG.debug("VM is missing, migration finished", - instance=instance) + _log(LOG.debug, "VM is missing, migration finished", + instance=instance) return libvirt.VIR_DOMAIN_JOB_COMPLETED else: - LOG.info("Error %(ex)s, migration failed", - {"ex": ex}, instance=instance) + _log(LOG.info, "Error %(ex)s, migration failed", {"ex": ex}, + instance=instance) return libvirt.VIR_DOMAIN_JOB_FAILED