Change in vdsm[ovirt-3.5]: vm: avoid race while setting guest cpu status

fromani at redhat.com fromani at redhat.com
Thu Sep 4 11:39:23 UTC 2014


Hello Dan Kenigsberg, Michal Skrivanek,

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

    http://gerrit.ovirt.org/32462

to review the following change.

Change subject: vm: avoid race while setting guest cpu status
......................................................................

vm: avoid race while setting guest cpu status

Inside the Vm class, the state swapping is not atomic.
This is because the reported state is function of two internal fields,
lastStatus and guestCpuRunning, which may be updated concurrently
and not atomically.

One clear example for this is BZ1111938, on which we see a
race between pause(), onLibvirtLifeCycle() and getStats(),
which makes a Vm incorrectly reported as 'Paused'
(instead of 'Saving State'), which confuses Engine.

To fix this, we make use of the guestCpuLock everywhere
we change the state of the guest CPU, in order to synchronize
access to the field with respect to pause() and cont() methods.

While good enough for the short term, more aggressive
refactoring is needed in this area.

Change-Id: I3aea96c7122d60e6cb888273678b565c3f3e537f
Bug-Url: https://bugzilla.redhat.com/1111938
Signed-off-by: Francesco Romani <fromani at redhat.com>
Reviewed-on: http://gerrit.ovirt.org/29113
Reviewed-by: Michal Skrivanek <michal.skrivanek at redhat.com>
Reviewed-by: Dan Kenigsberg <danken at redhat.com>
---
M tests/vmTests.py
M vdsm/virt/vm.py
2 files changed, 67 insertions(+), 12 deletions(-)


  git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/62/32462/1

diff --git a/tests/vmTests.py b/tests/vmTests.py
index 1a0a751..aa73a6e 100644
--- a/tests/vmTests.py
+++ b/tests/vmTests.py
@@ -24,12 +24,15 @@
 import re
 import shutil
 import tempfile
+import threading
+import time
 import xml.etree.ElementTree as ET
 
 import libvirt
 
 from virt import vm
 from virt import vmexitreason
+from virt import vmstatus
 from vdsm import constants
 from vdsm import define
 from testrunner import VdsmTestCase as TestCaseBase
@@ -42,6 +45,8 @@
 from vmTestsData import CONF_TO_DOMXML_PPC64
 from vmTestsData import CONF_TO_DOMXML_NO_VDSM
 
+from testValidation import slowtest
+
 
 class ConnectionMock:
     def __init__(self, *args):
@@ -52,6 +57,12 @@
 
     def listAllNetworks(self, *args):
         return []
+
+
+class FakeClientIF(object):
+    def __init__(self, *args, **kwargs):
+        self.channelListener = None
+        self.vmContainer = {}
 
 
 class FakeDomain(object):
@@ -902,17 +913,21 @@
 
 @contextmanager
 def FakeVM(params=None, devices=None, runCpu=False,
-           arch=caps.Architecture.X86_64):
+           arch=caps.Architecture.X86_64, status=None):
     with namedTemporaryDir() as tmpDir:
         with MonkeyPatchScope([(constants, 'P_VDSM_RUN', tmpDir + '/'),
                                (libvirtconnection, 'get', ConnectionMock)]):
             vmParams = {'vmId': 'TESTING'}
             vmParams.update({} if params is None else params)
-            fake = vm.Vm(None, vmParams)
+            cif = FakeClientIF()
+            fake = vm.Vm(cif, vmParams)
+            cif.vmContainer[fake.id] = fake
             fake.arch = arch
             fake.guestAgent = FakeGuestAgent()
             fake.conf['devices'] = [] if devices is None else devices
             fake._guestCpuRunning = runCpu
+            if status is not None:
+                fake._lastStatus = status
             yield fake
 
 
@@ -1311,3 +1326,32 @@
         with MonkeyPatchScope([(vm, '_listDomains',
                                 lambda: self._getAllDomains('novdsm'))]):
             self.assertFalse(vm.getVDSMDomains())
+
+
+class TestVmStatusTransitions(TestCaseBase):
+    @slowtest
+    def testSavingState(self):
+        with FakeVM(runCpu=True, status=vmstatus.UP) as vm:
+            vm._dom = FakeDomain(domState=libvirt.VIR_DOMAIN_RUNNING)
+
+            def _asyncEvent():
+                vm._onLibvirtLifecycleEvent(
+                    libvirt.VIR_DOMAIN_EVENT_SUSPENDED,
+                    -1, -1)
+
+            t = threading.Thread(target=_asyncEvent)
+            t.daemon = True
+
+            def _fireAsyncEvent(*args):
+                t.start()
+                time.sleep(0.5)
+                # pause the main thread to let the event one run
+
+            with MonkeyPatchScope([(vm, '_underlyingPause', _fireAsyncEvent)]):
+                self.assertEqual(vm.status()['status'], vmstatus.UP)
+                vm.pause(vmstatus.SAVING_STATE)
+                self.assertEqual(vm.status()['status'], vmstatus.SAVING_STATE)
+                t.join()
+                self.assertEqual(vm.status()['status'], vmstatus.SAVING_STATE)
+                # state must not change even after we are sure the event was
+                # handled
diff --git a/vdsm/virt/vm.py b/vdsm/virt/vm.py
index 9e4437a..91b7400 100644
--- a/vdsm/virt/vm.py
+++ b/vdsm/virt/vm.py
@@ -2722,8 +2722,8 @@
                 self.log.error('cannot cont while %s', self.lastStatus)
                 return errCode['unexpected']
             self._underlyingCont()
-            if hasattr(self, 'updateGuestCpuRunning'):
-                self.updateGuestCpuRunning()
+            self._setGuestCpuRunning(self._isDomainRunning(),
+                                     guestCpuLocked=True)
             self._lastStatus = afterState
             try:
                 with self._confLock:
@@ -2743,10 +2743,24 @@
             with self._confLock:
                 self.conf['pauseCode'] = pauseCode
             self._underlyingPause()
-            if hasattr(self, 'updateGuestCpuRunning'):
-                self.updateGuestCpuRunning()
+            self._setGuestCpuRunning(self._isDomainRunning(),
+                                     guestCpuLocked=True)
             self._lastStatus = afterState
             return {'status': doneCode, 'output': ['']}
+        finally:
+            if not guestCpuLocked:
+                self._guestCpuLock.release()
+
+    def _setGuestCpuRunning(self, isRunning, guestCpuLocked=False):
+        """
+        here we want to synchronize the access to guestCpuRunning
+        made by callback with the pause/cont methods.
+        To do so we reuse guestCpuLocked.
+        """
+        if not guestCpuLocked:
+            self._acquireCpuLockWithTimeout()
+        try:
+            self._guestCpuRunning = isRunning
         finally:
             if not guestCpuLocked:
                 self._guestCpuLock.release()
@@ -3173,9 +3187,6 @@
             return False
         else:
             return status[0] == libvirt.VIR_DOMAIN_RUNNING
-
-    def updateGuestCpuRunning(self):
-        self._guestCpuRunning = self._isDomainRunning()
 
     def _getUnderlyingVmDevicesInfo(self):
         """
@@ -4778,7 +4789,7 @@
             self.log.info('abnormal vm stop device %s error %s',
                           blockDevAlias, err)
             self.conf['pauseCode'] = err.upper()
-            self._guestCpuRunning = False
+            self._setGuestCpuRunning(False)
             if err.upper() == 'ENOSPC':
                 if not self.extendDrivesIfNeeded():
                     self.log.info("No VM drives were extended")
@@ -5475,7 +5486,7 @@
                         self._shutdownReason = vmexitreason.USER_SHUTDOWN
                 self._onQemuDeath()
         elif event == libvirt.VIR_DOMAIN_EVENT_SUSPENDED:
-            self._guestCpuRunning = False
+            self._setGuestCpuRunning(False)
             if detail == libvirt.VIR_DOMAIN_EVENT_SUSPENDED_PAUSED:
                 # Libvirt sometimes send the SUSPENDED/SUSPENDED_PAUSED event
                 # after RESUMED/RESUMED_MIGRATED (when VM status is PAUSED
@@ -5490,7 +5501,7 @@
                     hooks.after_vm_pause(domxml, self.conf)
 
         elif event == libvirt.VIR_DOMAIN_EVENT_RESUMED:
-            self._guestCpuRunning = True
+            self._setGuestCpuRunning(True)
             if detail == libvirt.VIR_DOMAIN_EVENT_RESUMED_UNPAUSED:
                 # This is not a real solution however the safest way to handle
                 # this for now. Ultimately we need to change the way how we are


-- 
To view, visit http://gerrit.ovirt.org/32462
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: newchange
Gerrit-Change-Id: I3aea96c7122d60e6cb888273678b565c3f3e537f
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: ovirt-3.5
Gerrit-Owner: Francesco Romani <fromani at redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken at redhat.com>
Gerrit-Reviewer: Michal Skrivanek <michal.skrivanek at redhat.com>


More information about the vdsm-patches mailing list