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