Change in vdsm[master]: fileSD: fix deleteImage flow

ishaby at redhat.com ishaby at redhat.com
Tue Dec 22 12:14:39 UTC 2015


Idan Shaby has posted comments on this change.

Change subject: fileSD: fix deleteImage flow
......................................................................


Patch Set 1:

(5 comments)

https://gerrit.ovirt.org/#/c/50804/1//COMMIT_MSG
Commit Message:

Line 14: 
Line 15: Also, if one of the files couldn't be removed, we didn't try to remove
Line 16: the others.
Line 17: Now we handle each removal separately, so that a failure in the removal
Line 18: of one file doesn't affect the removal of the others.
> Please describe also fixing handling of missing files, logging a warning in
Can't see a reason to describe here each and every case of the errors we print (the code is very simple), but I will add a few words just to make it even more clear.
Line 19: 
Line 20: Bug-Url: https://bugzilla.redhat.com/1292092
Line 21: Change-Id: I9eeb28a70f708a4f9a5effe4ff294da63b757369


https://gerrit.ovirt.org/#/c/50804/1/vdsm/storage/fileSD.py
File vdsm/storage/fileSD.py:

Line 223:         except OSError as e:
Line 224:             self.log.error("removed image dir: %s can't be removed", toDelDir)
Line 225:             raise se.ImageDeleteError("%s %s" % (imgUUID, str(e)))
Line 226: 
Line 227:     def _deleteFile(self, fileType, filePath):
> - Rename _deleteFile to _deleteVolumeFile. This is not a generic utility fo
Done
Line 228:         try:
Line 229:             self.oop.os.remove(filePath)
Line 230:             self.log.debug("%s file: %s was removed.", fileType, filePath)
Line 231:         except OSError as ose:


Line 226: 
Line 227:     def _deleteFile(self, fileType, filePath):
Line 228:         try:
Line 229:             self.oop.os.remove(filePath)
Line 230:             self.log.debug("%s file: %s was removed.", fileType, filePath)
> Put the log *before* the operation - if the operation get stuck, we want to
Done
Line 231:         except OSError as ose:
Line 232:             if ose.errno != errno.ENOENT:
Line 233:                 self.log.error("%s file: %s can't be removed.",
Line 234:                                fileType, filePath, exc_info=True)


Line 227:     def _deleteFile(self, fileType, filePath):
Line 228:         try:
Line 229:             self.oop.os.remove(filePath)
Line 230:             self.log.debug("%s file: %s was removed.", fileType, filePath)
Line 231:         except OSError as ose:
> Don't use such names, in this context there is only *one* exception, and th
Done
Line 232:             if ose.errno != errno.ENOENT:
Line 233:                 self.log.error("%s file: %s can't be removed.",
Line 234:                                fileType, filePath, exc_info=True)
Line 235: 


Line 230:             self.log.debug("%s file: %s was removed.", fileType, filePath)
Line 231:         except OSError as ose:
Line 232:             if ose.errno != errno.ENOENT:
Line 233:                 self.log.error("%s file: %s can't be removed.",
Line 234:                                fileType, filePath, exc_info=True)
> Do not use exc_info, it is non-standard and we want to kill it.
Done
Line 235: 
Line 236:     def getAllVolumes(self):
Line 237:         """
Line 238:         Return dict {volUUID: ((imgUUIDs,), parentUUID)} of the domain.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I9eeb28a70f708a4f9a5effe4ff294da63b757369
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Idan Shaby <ishaby at redhat.com>
Gerrit-Reviewer: Adam Litke <alitke at redhat.com>
Gerrit-Reviewer: Allon Mureinik <amureini at redhat.com>
Gerrit-Reviewer: Freddy Rolland <frolland at redhat.com>
Gerrit-Reviewer: Idan Shaby <ishaby at redhat.com>
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer <nsoffer at redhat.com>
Gerrit-Reviewer: Tal Nisan <tnisan at redhat.com>
Gerrit-Reviewer: gerrit-hooks <automation at ovirt.org>
Gerrit-HasComments: Yes


More information about the vdsm-patches mailing list