Change in vdsm[master]: vm: safer removal of the recovery file

fromani at redhat.com fromani at redhat.com
Wed Jan 13 10:57:36 UTC 2016


Francesco Romani has posted comments on this change.

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


Patch Set 3:

(3 comments)

thanks for pointing out more english mistakes :)

https://gerrit.ovirt.org/#/c/51387/3/vdsm/virt/vm.py
File vdsm/virt/vm.py:

Line 827:             with tempfile.NamedTemporaryFile(dir=constants.P_VDSM_RUN,
Line 828:                                              delete=False) as f:
Line 829:                 pickle.dump(vmState, f)
Line 830: 
Line 831:             os.rename(f.name, self._recoveryFile)
> Shouldn't the file writing be inside `with self._recoveryLock' above as wel
To guard against concurrent saveState() calls, sure.
It was made this way on purpose, to guard "only" against concurrent saveState() and _recoveryFile removal.
The problem here is the possibly expensive (even blocking?) I/O operations, we had nasty experiences in the past, but maybe I'm overly cautious here.
Other than that, no reason to have the I/O outside the lock.
Will think about it a bit more.
Line 832:         else:
Line 833:             self.log.debug('saveState after cleanup for status=%s',
Line 834:                            self.lastStatus)
Line 835: 


Line 1716:     def _cleanupRecoveryFile(self):
Line 1717:         with self._recoveryLock:
Line 1718:             recovery_file = self._recoveryFile
Line 1719:             self._recoveryFile = None
Line 1720:         if recovery_file:
> We should be consistent with using `is not None' at least within the patch.
good point!
Line 1721:             utils.rmFile(self._recoveryFile)
Line 1722: 
Line 1723:     def _cleanupStatsCache(self):
Line 1724:         try:


Line 1717:         with self._recoveryLock:
Line 1718:             recovery_file = self._recoveryFile
Line 1719:             self._recoveryFile = None
Line 1720:         if recovery_file:
Line 1721:             utils.rmFile(self._recoveryFile)
> Don't forget about this one.
sure, same reason as above why it is outside the lock.
Line 1722: 
Line 1723:     def _cleanupStatsCache(self):
Line 1724:         try:
Line 1725:             sampling.stats_cache.remove(self.id)


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I9a8d2b34599da17bee35c9bb694122ecd9d58068
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Francesco Romani <fromani at redhat.com>
Gerrit-Reviewer: Francesco Romani <fromani at redhat.com>
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Polednik <mpolednik at redhat.com>
Gerrit-Reviewer: Milan Zamazal <mzamazal at redhat.com>
Gerrit-Reviewer: Vinzenz Feenstra <vfeenstr at redhat.com>
Gerrit-Reviewer: gerrit-hooks <automation at ovirt.org>
Gerrit-HasComments: Yes


More information about the vdsm-patches mailing list