Change in vdsm[ovirt-3.6]: vm: safer removal of the recovery file

fromani at redhat.com fromani at redhat.com
Wed Jan 20 14:53:35 UTC 2016


Hello Dan Kenigsberg, Milan Zamazal,

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

    https://gerrit.ovirt.org/52515

to review the following change.

Change subject: vm: safer removal of the recovery file
......................................................................

vm: safer removal of the recovery file

If a user shutdowns a VM from inside the guest, Vdsm leaves
a stale recovery file.
The cause is a race between onQemuDeath, being asynchronous, and
Vm.destroy().
The actual culprit is the ordering of events between onQemuDeath and
destroy(), and the shutdown from within the guest is just the easiest
way to trigger this issue.

In the QEMU death path, Vdsm calls releaseVm, which wipes the recovery
file, and after a succesful completion does saveState, which recreates
the recovery file.
Later, on destroy(), Vdsm tries to do releaseVm again, but releaseVm
will not run, because of the flag that the previous succesful
call set. releaseVm must run exactly once.

Correct sequence:
- destroy()
-- vm._destroyed = True
-- releaseVm() - done (vm._released == False)
-- vm._released = True
[later]
- onQemuDeath
-- releaseVm() - skipped (vm._released == True)
-- saveState() - skipped (vm._destroyed == True)

Faulty sequence:
- onQemuDeath()
-- releaseVm() - done (vm._released == False)
-- vm._released = True
-- saveState() - done <BUG!> (vm._destroyed == False)
[later]
- destroy()
-- vm._destroyed = True
-- releaseVm() - skipped (vm._released == True)

The fix is to protect the recoveryFile handling with a lock.

Change-Id: I9a8d2b34599da17bee35c9bb694122ecd9d58068
Backport-To: 3.6
Backport-To: 3.5
Bug-Url: https://bugzilla.redhat.com/1253043
Signed-off-by: Francesco Romani <fromani at redhat.com>
Reviewed-on: https://gerrit.ovirt.org/51387
Continuous-Integration: Jenkins CI
Reviewed-by: Milan Zamazal <mzamazal at redhat.com>
Reviewed-by: Dan Kenigsberg <danken at redhat.com>
---
M vdsm/virt/vm.py
1 file changed, 17 insertions(+), 10 deletions(-)


  git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/15/52515/1

diff --git a/vdsm/virt/vm.py b/vdsm/virt/vm.py
index 98b8a16..df3f5dd 100644
--- a/vdsm/virt/vm.py
+++ b/vdsm/virt/vm.py
@@ -269,6 +269,7 @@
         self.cif = cif
         self.log = SimpleLogAdapter(self.log, {"vmId": self.conf['vmId']})
         self._destroyed = False
+        self._recoveryLock = threading.Lock()
         self._recoveryFile = constants.P_VDSM_RUN + \
             str(self.conf['vmId']) + '.recovery'
         self._monitorResponse = 0
@@ -815,16 +816,24 @@
         return base * (doubler + load) / doubler
 
     def saveState(self):
-        self._saveStateInternal()
+        with self._recoveryLock:
+            if self._recoveryFile is not None:
+                with tempfile.NamedTemporaryFile(dir=constants.P_VDSM_RUN,
+                                                 delete=False) as f:
+                    pickle.dump(self._getVmState(), f)
+
+                os.rename(f.name, self._recoveryFile)
+            else:
+                self.log.debug('saveState after cleanup for status=%s',
+                               self.lastStatus)
+
         try:
             self._updateDomainDescriptor()
         except Exception:
             # we do not care if _dom suddenly died now
             pass
 
-    def _saveStateInternal(self):
-        if self._destroyed:
-            return
+    def _getVmState(self):
         toSave = self.status()
         toSave['startTime'] = self._startTime
         if self.lastStatus != vmstatus.DOWN:
@@ -849,11 +858,7 @@
         toSave['_blockJobs'] = utils.picklecopy(
             self.conf.get('_blockJobs', {}))
 
-        with tempfile.NamedTemporaryFile(dir=constants.P_VDSM_RUN,
-                                         delete=False) as f:
-            pickle.dump(toSave, f)
-
-        os.rename(f.name, self._recoveryFile)
+        return toSave
 
     def onReboot(self):
         try:
@@ -1693,7 +1698,9 @@
             con.cleanup()
 
     def _cleanupRecoveryFile(self):
-        utils.rmFile(self._recoveryFile)
+        with self._recoveryLock:
+            utils.rmFile(self._recoveryFile)
+            self._recoveryFile = None
 
     def _cleanupStatsCache(self):
         try:


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

Gerrit-MessageType: newchange
Gerrit-Change-Id: I9a8d2b34599da17bee35c9bb694122ecd9d58068
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: ovirt-3.6
Gerrit-Owner: Francesco Romani <fromani at redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken at redhat.com>
Gerrit-Reviewer: Milan Zamazal <mzamazal at redhat.com>


More information about the vdsm-patches mailing list