Change in vdsm[master]: Improve logging on some filesystem operations

nsoffer at redhat.com nsoffer at redhat.com
Wed Mar 26 20:32:11 UTC 2014


Nir Soffer has posted comments on this change.

Change subject: Improve logging on some filesystem operations
......................................................................


Patch Set 2:

(10 comments)

I agree with Allon, that we better have the logs *before* the operation, and not after the operation.

If the operation blocks, we will have a clue about what we were doing (e.g. last message seen was creating a symlink).

When the operation fails, we get an error log or an exception is raised, so another log is not needed.

If the operation succeeded, we usually do not need a log, except if we like to know how much time the operation took, but I don't see such need currently.

Note also the capitalization - our logs are mostly capitalized.

http://gerrit.ovirt.org/#/c/26046/2/vdsm/storage/blockVolume.py
File vdsm/storage/blockVolume.py:

Line 434:             self.validateImagePath()
Line 435:         volPath = os.path.join(self.imagePath, self.volUUID)
Line 436:         if not os.path.lexists(volPath):
Line 437:             os.symlink(lvm.lvPath(self.sdUUID, self.volUUID), volPath)
Line 438:             self.log.debug("Creating symlink to %s", volPath)
Here we need the src path in the log, which will make the code easier to read:

    lvPath = lvm.lvPath(self.sdUUID, self.volUUID)
    self.log.debug("Creating symlink from %s to %s", lvPath, volPath)
Line 439:         self.volumePath = volPath
Line 440: 
Line 441:     def getVolumeTag(self, tagPrefix):
Line 442:         return _getVolumeTag(self.sdUUID, self.volUUID, tagPrefix)


http://gerrit.ovirt.org/#/c/26046/2/vdsm/storage/fileSD.py
File vdsm/storage/fileSD.py:

Line 345:     def deleteImage(self, sdUUID, imgUUID, volsImgs):
Line 346:         currImgDir = self.getImagePath(imgUUID)
Line 347:         dirName, baseName = os.path.split(currImgDir)
Line 348:         toDelDir = os.tempnam(dirName, sd.REMOVED_IMAGE_PREFIX + baseName)
Line 349:         try:
Lets log this also
Line 350:             self.oop.os.rename(currImgDir, toDelDir)
Line 351:         except OSError as e:
Line 352:             self.log.error("image: %s can't be moved", currImgDir)
Line 353:             raise se.ImageDeleteError("%s %s" % (imgUUID, str(e)))


Line 356:             try:
Line 357:                 self.oop.os.remove(volPath)
Line 358:                 self.oop.os.remove(volPath + '.meta')
Line 359:                 self.oop.os.remove(volPath + '.lease')
Line 360:                 self.log.debug("Removed volume: %s", volPath)
Consider adding log for each oop operation. If one on them got stuck, it will be more clear.
Line 361:             except OSError:
Line 362:                 self.log.error("vol: %s can't be removed.",
Line 363:                                volPath, exc_info=True)
Line 364:         try:


Line 360:                 self.log.debug("Removed volume: %s", volPath)
Line 361:             except OSError:
Line 362:                 self.log.error("vol: %s can't be removed.",
Line 363:                                volPath, exc_info=True)
Line 364:         try:
And this one also
Line 365:             self.oop.os.rmdir(toDelDir)
Line 366:         except OSError as e:
Line 367:             self.log.error("removed image dir: %s can't be removed", toDelDir)
Line 368:             raise se.ImageDeleteError("%s %s" % (imgUUID, str(e)))


Line 442:         imgRunDir = os.path.join(sdRunDir, imgUUID)
Line 443:         try:
Line 444:             os.symlink(srcImgPath, imgRunDir)
Line 445:             self.log.debug("Created img run dir from %s to %s", srcImgPath,
Line 446:                            imgRunDir)
This should use the same log format as other symlinks: "Creating symlink from %s to %s".
Line 447:         except OSError as e:
Line 448:             if e.errno == errno.EEXIST:
Line 449:                 self.log.debug("img run dir already exists: %s", imgRunDir)
Line 450:             else:


Line 464:         # (ordered volUUIDs) we fix them all.
Line 465:         imgDir = os.path.join(self.mountpoint, self.sdUUID, sd.DOMAIN_IMAGES,
Line 466:                               imgUUID)
Line 467:         volPaths = tuple(os.path.join(imgDir, v) for v in volUUIDs)
Line 468:         for volPath in volPaths:
This looks log worthy - "Fixing permissions on %s" - what do you think?
Line 469:             self.oop.fileUtils.copyUserModeToGroup(volPath)
Line 470: 
Line 471:         return self.createImageLinks(imgDir, imgUUID)
Line 472: 


Line 606:             for volFile in volFiles:
Line 607:                 tLink = os.path.join(basePath, rImg, volFile)
Line 608:                 tVol = os.path.join(basePath, templateImage, volFile)
Line 609:                 self.oop.utils.forceLink(tVol, tLink)
Line 610:                 self.log.debug("Relinking %s to %s", tVol, tLink)
Lets use the term "Force linking %s to %f", and "Linking %s to %s" for standard links.
Line 611: 
Line 612: 
Line 613: def getMountsList(pattern="*"):
Line 614:     finalPat = os.path.join(sd.StorageDomain.storage_repository,


http://gerrit.ovirt.org/#/c/26046/2/vdsm/storage/fileVolume.py
File vdsm/storage/fileVolume.py:

Line 285:         """
Line 286:         metaPath = self._getMetaVolumePath()
Line 287:         if self.oop.os.path.lexists(metaPath):
Line 288:             self.oop.os.unlink(metaPath)
Line 289:             self.log.debug("Removing metadada: %s", metaPath)
I think the path already includes "metadata"
Line 290: 
Line 291:     def getMetadataId(self):
Line 292:         """
Line 293:         Get the metadata Id


http://gerrit.ovirt.org/#/c/26046/2/vdsm/storage/sp.py
File vdsm/storage/sp.py:

Line 1127:         tmp_link_name = os.path.join(self.storage_repository,
Line 1128:                                      str(uuid.uuid4()))
Line 1129:         os.symlink(src, tmp_link_name)     # make tmp_link
Line 1130:         os.rename(tmp_link_name, linkName)
Line 1131:         self.log.debug("Storage domain linked from %s to %s", src, linkName)
_linkStorageDomain is included in the logging message automatically, so we can use standard terms like "Linking %s to %s".
Line 1132: 
Line 1133:     @unsecured
Line 1134:     def _cleanupDomainLinks(self, domain):
Line 1135:         linkPath = os.path.join(self.poolPath, domain)


Line 1134:     def _cleanupDomainLinks(self, domain):
Line 1135:         linkPath = os.path.join(self.poolPath, domain)
Line 1136:         try:
Line 1137:             os.remove(linkPath)
Line 1138:             self.log.debug("Removed domain link: %s", linkPath)
Same as above.
Line 1139:         except (OSError, IOError):
Line 1140:             pass
Line 1141: 
Line 1142:     @unsecured


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I3602513af123951f71091c03f799e36ea759aa61
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Xavi Francisco <xfrancis at redhat.com>
Gerrit-Reviewer: Allon Mureinik <amureini at redhat.com>
Gerrit-Reviewer: Nir Soffer <nsoffer at redhat.com>
Gerrit-Reviewer: Xavi Francisco <xfrancis at redhat.com>
Gerrit-Reviewer: automation at ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes


More information about the vdsm-patches mailing list