Change in vdsm[master]: [WIP] Remove subChainSizeCalc() and getSubChain(). Remove ge...

Eduardo has uploaded a new change for review.

Change subject: [WIP] Remove subChainSizeCalc() and getSubChain(). Remove getAllChildrenList().
......................................................................

[WIP] Remove subChainSizeCalc() and getSubChain().
Remove getAllChildrenList().

Change-Id: Iffbbf71269eddb53c032c381d10e20b771c47c6b
Signed-off-by: Eduardo 
---
M vdsm/storage/blockVolume.py
M vdsm/storage/fileVolume.py
M vdsm/storage/image.py
3 files changed, 42 insertions(+), 112 deletions(-)

git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/68/3468/1
--
To view, visit http://gerrit.ovirt.org/3468
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: newchange
Gerrit-Change-Id: Iffbbf71269eddb53c032c381d10e20b771c47c6b
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Eduardo

Eduardo has posted comments on this change.

Change subject: [WIP] Remove subChainSizeCalc() and getSubChain(). Remove getAllChildrenList().
......................................................................

Patch Set 4: I would prefer that you didn't submit this

This is a clean up commit.
PLEASE IGNORE

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Iffbbf71269eddb53c032c381d10e20b771c47c6b
Gerrit-PatchSet: 4
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Eduardo 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Eduardo 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: Igor Lvovsky

[Igor Lvovsky]

Igor Lvovsky has posted comments on this change.

Change subject: [WIP] Remove subChainSizeCalc() and getSubChain(). Remove getAllChildrenList().
......................................................................

Patch Set 5: I would prefer that you didn't submit this

(4 inline comments)

....................................................
File vdsm/storage/image.py
Line 1034:         volsImgs = dict((k, v) for k, v in allVols.iteritems() if imgUUID == v[0])
Edu, please use normal variable names, not 'k' and 'v'.
I know that it's looks very trivial for you but not for us (at least not for me).
I don't want to recall every time how getAllVolumes() output looks like.

Line 1068: 
Ah, I don't know. You wanted to get rid from subChainSizeCalc and getSubChain.
and you just put their code inline. As the result we got a huge merge function.

Line 1081:                 chainToRemove = self._baseRawVolumeMerge(sdDom, srcVolParams, volParams, [vols[vUUID] for vUUID in chain])
1. What is vols[vUUID]? is it images? why?
2. split the lines, please

Line 1084:                 chainToRemove = self._baseCowVolumeMerge(sdDom, srcVolParams, volParams, chainSize, chain)
again, split the lines

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Iffbbf71269eddb53c032c381d10e20b771c47c6b
Gerrit-PatchSet: 5
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Eduardo 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Eduardo 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: Igor Lvovsky

Change in vdsm[master]: [WIP] BZ#688640 - Remove subChainSizeCalc() and getSubChain(...

[Igor Lvovsky]

Igor Lvovsky has posted comments on this change.

Change subject: [WIP] BZ#688640 - Remove subChainSizeCalc() and getSubChain(). Remove getAllChildrenList().
......................................................................

Patch Set 6: I would prefer that you didn't submit this

(4 inline comments)

....................................................
File vdsm/storage/image.py
Line 1037:             chainSize += vols[volUUID].getSize()
getVolumeSize ?

Line 1040:             chain.insert(0, ancestor)
you can start the chain with ansestor

Line 1043:         if chainSize > vols[ancestor].getSize():
s = vols[succesors].getVolumeSize()
if chainSize > s:
    chainSize = s

chainSize = int(chainSize * 1.1)

Line 1056:             volParams = vols[dstParentUUID].getVolumeParams()
if ansestor was son of template you didn't get it vols{}, so vols[dstParentUUID] will raise exception

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Iffbbf71269eddb53c032c381d10e20b771c47c6b
Gerrit-PatchSet: 6
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Eduardo 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Eduardo 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: Igor Lvovsky

Change in vdsm[master]: BZ#688640 - Remove subChainSizeCalc() and getSubChain(). Rem...

[Igor Lvovsky]

Igor Lvovsky has posted comments on this change.

Change subject: BZ#688640 - Remove subChainSizeCalc() and getSubChain(). Remove getAllChildrenList().
......................................................................

Patch Set 8: Looks good to me, but someone else must approve

(1 inline comment)

I hope it's works.

....................................................
File vdsm/storage/image.py
Line 1062: 
Ah, it looks fine, except the fact that now (when you flatted all functions inline) the merge looks like .....
I know that it was very complicated, but now it just an ugly beast.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Iffbbf71269eddb53c032c381d10e20b771c47c6b
Gerrit-PatchSet: 8
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Eduardo 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Eduardo 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: Igor Lvovsky

Change in vdsm[master]: BZ#688640 - Remove subChainSizeCalc() and getSubChain(). Rem...

Eduardo has posted comments on this change.

Change subject: BZ#688640 - Remove subChainSizeCalc() and getSubChain(). Remove getAllChildrenList().
......................................................................

Patch Set 8: (1 inline comment)

....................................................
File vdsm/storage/image.py
Line 1062: 
Is temporal.
We already agreed on simplify the merge.
Flatted only two, not "all".

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Iffbbf71269eddb53c032c381d10e20b771c47c6b
Gerrit-PatchSet: 8
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Eduardo 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Eduardo 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: Igor Lvovsky

Change in vdsm[master]: BZ#688640 - Remove subChainSizeCalc() and getSubChain(). Rem...

Ayal Baron has posted comments on this change.

Change subject: BZ#688640 - Remove subChainSizeCalc() and getSubChain(). Remove getAllChildrenList().
......................................................................

Patch Set 8: I would prefer that you didn't submit this

(12 inline comments)

....................................................
Commit Message
Line 8: Remove getAllChildrenList().
why?

....................................................
File vdsm/storage/image.py
Line 959:         srcVol = chain[-1]
s/srcVol/dstVol/

Line 1006:         rChain = [vol.volUUID for vol in chain if srcVol.volUUID != srcVolParams['volUUID']]
s/rChain/rmChain/

Line 1011:     # TODO: Review this and accessory functions
???

Line 1023:         # Filter volumes related to this image (without the eventual template)
comment is wrong and redundant

Line 1024:         volsImgs = dict((k, v) for k, v in allVols.iteritems()
as in previous patch, should be a function and variable name is wrong

Line 1033:         srcVol = vols[successor]
s/successor/something legible/
s/srcVol/dstVol/ ????????????????????

Line 1034:         srcVolParams = srcVol.getVolumeParams()
s/srcVolParams/dstVolParams/

Line 1036:         for vUUID, vol in vols.iteritems():
why vUUID here and vName elsewhere?

Line 1048:         volUUID = successor
s/volUUID/curVol/ ?

Line 1053:         else:
get rid of else
you can change while to:
endVol = vols[ancestor].getParent()
while (curVol != endVol):

Line 1057:         sz = volParams['size']
sz is usually string zero so not that good a name. why not 'imageApparentSize' ?
s/chainSize/chainRealSize/

requiredSpace = min(chainRealSize, imageApparentSize) * 1.1

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Iffbbf71269eddb53c032c381d10e20b771c47c6b
Gerrit-PatchSet: 8
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Eduardo 
Gerrit-Reviewer: Ayal Baron 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Eduardo 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: Igor Lvovsky

Change in vdsm[master]: BZ#688640 - Remove subChainSizeCalc() and getSubChain(). Rem...

[Igor Lvovsky]

Igor Lvovsky has posted comments on this change.

Change subject: BZ#688640 - Remove subChainSizeCalc() and getSubChain(). Remove getAllChildrenList().
......................................................................

Patch Set 9: Looks good to me, but someone else must approve

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Iffbbf71269eddb53c032c381d10e20b771c47c6b
Gerrit-PatchSet: 9
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Eduardo 
Gerrit-Reviewer: Ayal Baron 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Eduardo 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: Igor Lvovsky

Change in vdsm[master]: BZ#688640 - Remove subChainSizeCalc() and getSubChain(). Rem...

Shu Ming has posted comments on this change.

Change subject: BZ#688640 - Remove subChainSizeCalc() and getSubChain(). Remove getAllChildrenList().
......................................................................

Patch Set 13: I would prefer that you didn't submit this

Please correct the bugzilla number to 788640 in the commit message.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Iffbbf71269eddb53c032c381d10e20b771c47c6b
Gerrit-PatchSet: 13
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Eduardo 
Gerrit-Reviewer: Ayal Baron 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Eduardo 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: Igor Lvovsky 
Gerrit-Reviewer: Shu Ming

Change in vdsm[master]: BZ#688640 - Remove subChainSizeCalc() and getSubChain(). Rem...

[Haim Ateya]

Haim Ateya has posted comments on this change.

Change subject: BZ#688640 - Remove subChainSizeCalc() and getSubChain(). Remove getAllChildrenList().
......................................................................

Patch Set 13: Verified

tested extensively on both block & file domains running most of the core scenarios: copy, move, delete, import, export volumes\images.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Iffbbf71269eddb53c032c381d10e20b771c47c6b
Gerrit-PatchSet: 13
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Eduardo 
Gerrit-Reviewer: Ayal Baron 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Eduardo 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: Haim Ateya 
Gerrit-Reviewer: Igor Lvovsky 
Gerrit-Reviewer: Shu Ming

Change in vdsm[master]: BZ#688640 - Remove subChainSizeCalc() and getSubChain(). Rem...

[Dan Kenigsberg]

Dan Kenigsberg has posted comments on this change.

Change subject: BZ#688640 - Remove subChainSizeCalc() and getSubChain(). Remove getAllChildrenList().
......................................................................

Patch Set 14: (1 inline comment)

....................................................
File vdsm/storage/image.py
Line 1046:         chain = []
why did you inline .subChainSizeCalc()? do you care about the extra stack frame, or fear that merge() was too simple as it was?

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Iffbbf71269eddb53c032c381d10e20b771c47c6b
Gerrit-PatchSet: 14
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Eduardo 
Gerrit-Reviewer: Ayal Baron 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Eduardo 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: Haim Ateya 
Gerrit-Reviewer: Igor Lvovsky 
Gerrit-Reviewer: Shu Ming

Change in vdsm[master]: BZ#788640 - Remove subChainSizeCalc() and getSubChain(). Rem...

Ayal Baron has posted comments on this change.

Change subject: BZ#788640 - Remove subChainSizeCalc() and getSubChain(). Remove getAllChildrenList().
......................................................................

Patch Set 16: I would prefer that you didn't submit this

(1 inline comment)

....................................................
File vdsm/storage/image.py
Line 1056:             currVolName = vols[currVolName].getParent()
I agree with Dan, this should be in a separate function

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Iffbbf71269eddb53c032c381d10e20b771c47c6b
Gerrit-PatchSet: 16
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Eduardo 
Gerrit-Reviewer: Ayal Baron 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Eduardo 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: Haim Ateya 
Gerrit-Reviewer: Igor Lvovsky 
Gerrit-Reviewer: Shu Ming

Change in vdsm[master]: BZ#788640 - Remove subChainSizeCalc() and getSubChain(). Rem...

Ayal Baron has posted comments on this change.

Change subject: BZ#788640 - Remove subChainSizeCalc() and getSubChain(). Remove getAllChildrenList().
......................................................................

Patch Set 17: I would prefer that you didn't submit this

(1 inline comment)

....................................................
File vdsm/storage/image.py
Line 1036:         return min(accumulatedChainSize, imageApparentSize) * 1.1, chain
This function should just return accumulatedChainSize.
It is up to the calling function to determine the usage and add the extra 10%, it has nothing to do with the "actual" size calculation

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Iffbbf71269eddb53c032c381d10e20b771c47c6b
Gerrit-PatchSet: 17
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Eduardo 
Gerrit-Reviewer: Ayal Baron 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Eduardo 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: Haim Ateya 
Gerrit-Reviewer: Igor Lvovsky 
Gerrit-Reviewer: Shu Ming

Change in vdsm[master]: BZ#788640 - Remove subChainSizeCalc() and getSubChain(). Rem...

Ayal Baron has posted comments on this change.

Change subject: BZ#788640 - Remove subChainSizeCalc() and getSubChain(). Remove getAllChildrenList().
......................................................................

Patch Set 18: Looks good to me, approved

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Iffbbf71269eddb53c032c381d10e20b771c47c6b
Gerrit-PatchSet: 18
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Eduardo 
Gerrit-Reviewer: Ayal Baron 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Eduardo 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: Haim Ateya 
Gerrit-Reviewer: Igor Lvovsky 
Gerrit-Reviewer: Shu Ming

Change in vdsm[master]: BZ#788640 - Remove subChainSizeCalc() and getSubChain(). Rem...

Eduardo has posted comments on this change.

Change subject: BZ#788640 - Remove subChainSizeCalc() and getSubChain(). Remove getAllChildrenList().
......................................................................

Patch Set 18:

Previously verified by Hateya

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Iffbbf71269eddb53c032c381d10e20b771c47c6b
Gerrit-PatchSet: 18
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Eduardo 
Gerrit-Reviewer: Ayal Baron 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Eduardo 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: Haim Ateya 
Gerrit-Reviewer: Igor Lvovsky 
Gerrit-Reviewer: Shu Ming

Change in vdsm[master]: BZ#788640 - Remove subChainSizeCalc() and getSubChain(). Rem...

Eduardo has posted comments on this change.

Change subject: BZ#788640 - Remove subChainSizeCalc() and getSubChain(). Remove getAllChildrenList().
......................................................................

Patch Set 18: Verified

Previously verified by Hateya

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Iffbbf71269eddb53c032c381d10e20b771c47c6b
Gerrit-PatchSet: 18
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Eduardo 
Gerrit-Reviewer: Ayal Baron 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Eduardo 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: Haim Ateya 
Gerrit-Reviewer: Igor Lvovsky 
Gerrit-Reviewer: Shu Ming

Change in vdsm[master]: BZ#788640 - Remove subChainSizeCalc() and getSubChain(). Rem...

[Dan Kenigsberg]

Dan Kenigsberg has submitted this change and it was merged.

Change subject: BZ#788640 - Remove subChainSizeCalc() and getSubChain(). Remove getAllChildrenList().
......................................................................

BZ#788640 - Remove subChainSizeCalc() and getSubChain().
Remove getAllChildrenList().

getAllChildrenList() should be removed since it is intrinsically
race prone.
*subChain* functions were called only once.

Change-Id: Iffbbf71269eddb53c032c381d10e20b771c47c6b
Signed-off-by: Eduardo 
---
M vdsm/storage/blockVolume.py
M vdsm/storage/fileVolume.py
M vdsm/storage/image.py
3 files changed, 53 insertions(+), 115 deletions(-)

Approvals:
  Ayal Baron: Looks good to me, approved
  Eduardo: Verified
  Dan Kenigsberg:

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

Gerrit-MessageType: merged
Gerrit-Change-Id: Iffbbf71269eddb53c032c381d10e20b771c47c6b
Gerrit-PatchSet: 18
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Eduardo 
Gerrit-Reviewer: Ayal Baron 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Eduardo 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: Haim Ateya 
Gerrit-Reviewer: Igor Lvovsky 
Gerrit-Reviewer: Shu Ming