Change in vdsm[master]: Introducing the template activation leak.

Yaniv Bronhaim has posted comments on this change.

Change subject: Introducing the template activation leak.
......................................................................

Patch Set 4: (8 inline comments)

....................................................
File vdsm/storage/blockVolume.py
Line 377:         lvm.activateLVs(self.sdUUID, self.volUUID)
Line 378: 
Line 379:     def deactivate(self):
Line 380:         # TODO: use reference count here
Line 381:         lvm.deactivateLVs(self.sdUUID, self.volUUID)
Do you use deactivate?

Why do you want to add ref counter here also? all the reason of that is to skip resourceManager..
Line 382: 
Line 383:     @logskip("ResourceManager")
Line 384:     def llPrepare(self, rw=False, setrw=False):
Line 385:         """

Line 404:         If justme is false, the entire COW chain should be torn down.
Line 405:         """
Line 406:         cls.log.info("Tearing down volume %s/%s justme %s" % (sdUUID, volUUID, justme))
Line 407:         tagParentKey, tagParentSep, parentTail = TAG_PREFIX_PARENT.partition('_')
Line 408:         tagImageKey, tagImageSep, imageTail = TAG_PREFIX_IMAGE.partition('_')
It looks weird... I'd prefer 2 different constants for the separator sign and prefix key for Parent and Image. This way it simpler and easy to understand.
Line 409:         lvmActivationNamespace = sd.getNamespace(sdUUID, LVM_ACTIVATION_NAMESPACE)
Line 410:         # Will log a false positive error if volUUID is a template.
Line 411:         rmanager.releaseResource(lvmActivationNamespace, volUUID)
Line 412:         if not justme:

Line 406:         cls.log.info("Tearing down volume %s/%s justme %s" % (sdUUID, volUUID, justme))
Line 407:         tagParentKey, tagParentSep, parentTail = TAG_PREFIX_PARENT.partition('_')
Line 408:         tagImageKey, tagImageSep, imageTail = TAG_PREFIX_IMAGE.partition('_')
Line 409:         lvmActivationNamespace = sd.getNamespace(sdUUID, LVM_ACTIVATION_NAMESPACE)
Line 410:         # Will log a false positive error if volUUID is a template.
Who? Resource manager? The comment here needs more info..
Line 411:         rmanager.releaseResource(lvmActivationNamespace, volUUID)
Line 412:         if not justme:
Line 413:             try:
Line 414:                 tags = _getVolumeTags(sdUUID, volUUID, tagParentSep)

Line 410:         # Will log a false positive error if volUUID is a template.
Line 411:         rmanager.releaseResource(lvmActivationNamespace, volUUID)
Line 412:         if not justme:
Line 413:             try:
Line 414:                 tags = _getVolumeTags(sdUUID, volUUID, tagParentSep)
If you add default value for the same value you pass here, why do you do all that unnecessary operations to get this separator?
Line 415:             except Exception, e:
Line 416:                 # If storage not accessible or lvm error occurred
Line 417:                 # we will failure to get the parent volume.
Line 418:                 # We can live with it and still succeed in volume's teardown.

Line 418:                 # We can live with it and still succeed in volume's teardown.
Line 419:                 pvolUUID = volume.BLANK_UUID
Line 420:                 cls.log.warn("Failure to get parent of volume %s/%s (%s)" % (sdUUID, volUUID, e))
Line 421: 
Line 422:             pvolUUID = tags[tagParentKey]
same.. you can have constant for tagParentKey and tagImageKey, it's never changed...
Line 423:             imgUUID = tags[tagImageKey]
Line 424:             if pvolUUID != volume.BLANK_UUID:
Line 425:                 ptags = _getVolumeTags(sdUUID, pvolUUID, tagParentSep)
Line 426:                 pimgUUID = ptags[tagImageKey]

Line 484:                     imgUUIDs.remove(imgUUID)
Line 485: 
Line 486:         return imgUUIDs
Line 487: 
Line 488:     def getVolumeTag(self, tagPrefix):
1. same here, with '_' (separator) constant it is much simpler.
 2. this function should look like: 
def getVolumeTag(self, tagPrefix):
 return _getVolumeTags(self.sdUUID, self.volUUID)[tagPrefix]

(using sep=SPERATOR_SIGN as default value to _getVolumeTags)

If you want, you can verify inside that this value exist like you do here, but I think it should be handled by the caller..
Line 489:         # ('PU', '_', '') = "PU_".partition('_')
Line 490:         tagKey, tagSep, tail = tagPrefix.partition('_')
Line 491:         tags = _getVolumeTags(self.sdUUID, self.volUUID, tagSep)
Line 492:         try:

....................................................
File vdsm/storage/volume.py
Line 547: 
Line 548:             if not chainrw and rw and self.isInternal() and setrw and not self.recheckIfLeaf():
Line 549:                 raise se.InternalVolumeNonWritable(self)
Line 550: 
Line 551:         if not isTemplate:
I agree but I'd add comment here that explains that if volume isShared we only activate lv instead of calling prepare function that does reference counting..
Line 552:             self.llPrepare(rw=rw, setrw=setrw)
Line 553:             try:
Line 554:                 # Mtime is the time of the last prepare for RW
Line 555:                 if rw:

Line 564:                 self.teardown(self.sdUUID, self.volUUID)
Line 565:                 raise e
Line 566:         # Do not take locks on templates at all, but activate them.
Line 567:         elif self.__class__.__name__ == "BlockVolume":
Line 568:             self.activate()
Can't you just check if isShared() in blockVolume::teardown before you release the resource?
Line 569: 
Line 570:         return True
Line 571: 
Line 572:     @classmethod

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ieedf863ac967f34405f038201bac324c52fbbe89
Gerrit-PatchSet: 4
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Eduardo 
Gerrit-Reviewer: Ayal Baron 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Eduardo 
Gerrit-Reviewer: Igor Lvovsky 
Gerrit-Reviewer: Yaniv Bronhaim