Hello Dan Kenigsberg,

I'd like you to do a code review.  Please visit

    https://gerrit.ovirt.org/45379

to review the following change.

Change subject: vm: make acpiShutdown handle NotConnectedError
......................................................................

vm: make acpiShutdown handle NotConnectedError

In the Vm shutdown flow it is possible that the domain is
disconnected asynchronously as in any other Vm flow.

This patch makes Vm.acpiShutdown() handle the
sudden domain disconnections in a graceful manner.

vmpowerdown needs to be aware if acpiShutdown failed
for this reason, so it can go ahead with the callback
chain without wasteful waits,

Change-Id: I244f00d62ee24fb42ba3d654961a8fc22f4a6c25
Backport-To: 3.6
Bug-Url: https://bugzilla.redhat.com/1154389
Signed-off-by: Francesco Romani <[email protected]>
Reviewed-on: https://gerrit.ovirt.org/34879
Continuous-Integration: Jenkins CI
Reviewed-by: Dan Kenigsberg <[email protected]>
---
M tests/vmTests.py
M tests/vmfakelib.py
M vdsm/virt/vm.py
M vdsm/virt/vmpowerdown.py
4 files changed, 28 insertions(+), 2 deletions(-)


  git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/79/45379/1

diff --git a/tests/vmTests.py b/tests/vmTests.py
index a6b631c..1ac1ea7 100644
--- a/tests/vmTests.py
+++ b/tests/vmTests.py
@@ -1201,6 +1201,16 @@
             self.assertEqual(res,
                              response.error('ticketErr', message))
 
+    def testAcpiShutdownDisconnected(self):
+        with fake.VM() as testvm:
+            testvm._dom = virdomain.Disconnected(vmid='testvm')
+            self.assertTrue(response.is_error(testvm.acpiShutdown()))
+
+    def testAcpiShutdownConnected(self):
+        with fake.VM() as testvm:
+            testvm._dom = fake.Domain(vmId='testvm')
+            self.assertFalse(response.is_error(testvm.acpiShutdown()))
+
 
 class ChangingSchedulerDomain(object):
 
diff --git a/tests/vmfakelib.py b/tests/vmfakelib.py
index 83f966b..52ef07a 100644
--- a/tests/vmfakelib.py
+++ b/tests/vmfakelib.py
@@ -257,6 +257,9 @@
         self._failIfRequested()
         return 3  # thawed filesystems
 
+    def shutdownFlags(self, flags):
+        pass
+
 
 class GuestAgent(object):
     def __init__(self):
diff --git a/vdsm/virt/vm.py b/vdsm/virt/vm.py
index a354312..5ea7cd1 100644
--- a/vdsm/virt/vm.py
+++ b/vdsm/virt/vm.py
@@ -3744,7 +3744,16 @@
     def acpiShutdown(self):
         with self._shutdownLock:
             self._shutdownReason = vmexitreason.ADMIN_SHUTDOWN
-        self._dom.shutdownFlags(libvirt.VIR_DOMAIN_SHUTDOWN_ACPI_POWER_BTN)
+        try:
+            self._dom.shutdownFlags(libvirt.VIR_DOMAIN_SHUTDOWN_ACPI_POWER_BTN)
+        except virdomain.NotConnectedError:
+            # the VM was already shut off asynchronously,
+            # so ignore error and quickly exit
+            self.log.warning('failed to invoke acpiShutdown: '
+                             'domain not connected')
+            return response.error('down')
+        else:
+            return response.success()
 
     def setBalloonTarget(self, target):
 
diff --git a/vdsm/virt/vmpowerdown.py b/vdsm/virt/vmpowerdown.py
index 2382a88..ffa10c9 100644
--- a/vdsm/virt/vmpowerdown.py
+++ b/vdsm/virt/vmpowerdown.py
@@ -19,6 +19,7 @@
 #
 import libvirt
 
+from vdsm import response
 from vdsm import utils
 from vdsm.define import doneCode, errCode
 
@@ -88,7 +89,8 @@
         return self.event.wait(self.delay + self.timeout)
 
     def acpiCallback(self):
-        self.vm.acpiShutdown()
+        if response.is_error(self.vm.acpiShutdown()):
+            return False
         return self.event.wait(self.timeout)
 
     def forceCallback(self):
@@ -104,9 +106,11 @@
         return self.event.wait(self.delay + self.timeout)
 
     def acpiCallback(self):
+        # TODO: fix like acpiShutdown
         self.vm._dom.reboot(libvirt.VIR_DOMAIN_REBOOT_ACPI_POWER_BTN)
         return self.event.wait(self.timeout)
 
     def forceCallback(self):
+        # TODO: fix like acpiShutdown
         self.vm._dom.reset(0)
         return self.event.wait(self.timeout)


-- 
To view, visit https://gerrit.ovirt.org/45379
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: newchange
Gerrit-Change-Id: I244f00d62ee24fb42ba3d654961a8fc22f4a6c25
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: ovirt-3.6
Gerrit-Owner: Francesco Romani <[email protected]>
Gerrit-Reviewer: Dan Kenigsberg <[email protected]>
_______________________________________________
vdsm-patches mailing list
[email protected]
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches

Reply via email to