7:51 a.m.
Eduardo has uploaded a new change for review.
Change subject: Avoid template deactivation and lock.
......................................................................
Avoid template deactivation and lock.
Change-Id: Ieedf863ac967f34405f038201bac324c52fbbe89
---
M vdsm/storage/blockVolume.py
M vdsm/storage/volume.py
2 files changed, 39 insertions(+), 18 deletions(-)
git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/63/863/1
--
To view, visit http://gerrit.ovirt.org/863
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: Ieedf863ac967f34405f038201bac324c52fbbe89
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Eduardo <ewarszaw(a)redhat.com>
8:05 a.m.
New subject: Change in vdsm[master]: Introducing the template activation leak.
Itamar Heim has posted comments on this change.
Change subject: Introducing the template activation leak.
......................................................................
Patch Set 4:
pinf
--
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 <ewarszaw(a)redhat.com>
Gerrit-Reviewer: Ayal Baron <abaron(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Eduardo <ewarszaw(a)redhat.com>
Gerrit-Reviewer: Igor Lvovsky <ilvovsky(a)redhat.com>
Gerrit-Reviewer: Itamar Heim <iheim(a)redhat.com>
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei(a)redhat.com>
Gerrit-HasComments: No
3:13 a.m.
New subject: Change in vdsm[master]: Introducing the template activation leak.
Igor Lvovsky has posted comments on this change.
Change subject: Introducing the template activation leak.
......................................................................
Patch Set 4: Looks good to me, but someone else must approve
--
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 <ewarszaw(a)redhat.com>
Gerrit-Reviewer: Ayal Baron <abaron(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Eduardo <ewarszaw(a)redhat.com>
Gerrit-Reviewer: Igor Lvovsky <ilvovsky(a)redhat.com>
2:03 a.m.
New subject: Change in vdsm[master]: Introducing the template activation leak.
Ayal Baron has posted comments on this change.
Change subject: Introducing the template activation leak.
......................................................................
Patch Set 3: I would prefer that you didn't submit this
(1 inline comment)
This and the parent patch should be 1 patch. This separation makes no sense and make the
process lengthy and dumb.
....................................................
File vdsm/storage/blockVolume.py
Line 426: pimgUUID = ptags[TAG_PREFIX_IMAGE]
s/TAG_PREFIX_IMAGE/tagImageKey/
--
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: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Eduardo <ewarszaw(a)redhat.com>
Gerrit-Reviewer: Ayal Baron <abaron(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Eduardo <ewarszaw(a)redhat.com>
Gerrit-Reviewer: Igor Lvovsky <ilvovsky(a)redhat.com>
2:34 a.m.
New subject: Change in vdsm[master]: Introducing the template activation leak.
Igor Lvovsky has posted comments on this change.
Change subject: Introducing the template activation leak.
......................................................................
Patch Set 2: I would prefer that you didn't submit this
(3 inline comments)
....................................................
File vdsm/storage/blockVolume.py
Line 420: pvolUUID = tags[TAG_PREFIX_PARENT]
Should be without '_' in TAG_PREFIX_PARENT
Line 421: imgUUID = tags[TAG_PREFIX_IMAGE]
Same
....................................................
File vdsm/storage/volume.py
Line 551: if not isTemplate:
Please add comment to explain
--
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: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Eduardo <ewarszaw(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Eduardo <ewarszaw(a)redhat.com>
Gerrit-Reviewer: Igor Lvovsky <ilvovsky(a)redhat.com>
10:03 a.m.
Dan Kenigsberg has posted comments on this change.
Change subject: Avoid template deactivation and lock.
......................................................................
Patch Set 1: I would prefer that you didn't submit this
(4 inline comments)
....................................................
Commit Message
Line 8:
describe the intention:
We would like to avoid taking re-acquiring resources for Vm's disks on Vdsm restart.
However, we want to keep the template active after it is used by an spm verb. Instead of
counting the references to the template and deactivating when the refcount is 0, we leak
the activation on first use and never deactivate the template.
this may lead to performance hit once we have thousands of templates.
....................................................
File vdsm/storage/blockVolume.py
Line 408: # Will log a false positive error for templates.
if volUUID is a template
Line 425: # A parent from other image is a template.
other->another
Line 489: return tags[tagPrefix[:-1]]
into previous patch
--
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: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Eduardo <ewarszaw(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
6:15 a.m.
New subject: 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 <ewarszaw(a)redhat.com>
Gerrit-Reviewer: Ayal Baron <abaron(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Eduardo <ewarszaw(a)redhat.com>
Gerrit-Reviewer: Igor Lvovsky <ilvovsky(a)redhat.com>
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei(a)redhat.com>
8:32 a.m.
New subject: Change in vdsm[master]: Introducing the template activation leak.
Itamar Heim has abandoned this change.
Change subject: Introducing the template activation leak.
......................................................................
Abandoned
abandoning - old. no reply. not touched for a while. please restore if relevant
--
To view, visit http://gerrit.ovirt.org/863
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: abandon
Gerrit-Change-Id: Ieedf863ac967f34405f038201bac324c52fbbe89
Gerrit-PatchSet: 4
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Eduardo <ewarszaw(a)gmail.com>
Gerrit-Reviewer: Ayal Baron <abaron(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Eduardo <ewarszaw(a)gmail.com>
Gerrit-Reviewer: Igor Lvovsky <ilvovsky(a)redhat.com>
Gerrit-Reviewer: Itamar Heim <iheim(a)redhat.com>
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei(a)redhat.com>
8:47 a.m.
New subject: Change in vdsm[master]: Introducing the template activation leak.
Ayal Baron has posted comments on this change.
Change subject: Introducing the template activation leak.
......................................................................
Patch Set 4: Looks good to me, approved
--
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 <ewarszaw(a)redhat.com>
Gerrit-Reviewer: Ayal Baron <abaron(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Eduardo <ewarszaw(a)redhat.com>
Gerrit-Reviewer: Igor Lvovsky <ilvovsky(a)redhat.com>
10:24 a.m.
Igor Lvovsky has posted comments on this change.
Change subject: Avoid template deactivation and lock.
......................................................................
Patch Set 1: I would prefer that you didn't submit this
(1 inline comment)
....................................................
File vdsm/storage/blockVolume.py
Line 489: return tags[tagPrefix[:-1]]
do it on _getVolumeTags
--
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: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Eduardo <ewarszaw(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Igor Lvovsky <ilvovsky(a)redhat.com>
3649
days inactive
4495
days old
vdsm-patches@lists.fedorahosted.org
9 comments
6 participants
participants (6)
-
abaron＠redhat.com -
Dan Kenigsberg -
ewarszaw＠redhat.com -
Igor Lvovsky -
iheim＠redhat.com -
ybronhei＠redhat.com