Change in vdsm[master]: [WIP] Added blockSD.getImageMap(), [block|file]SD.getAllVolu...

abaron at redhat.com abaron at redhat.com
Mon Apr 23 09:39:18 UTC 2012


Ayal Baron has posted comments on this change.

Change subject: [WIP] Added blockSD.getImageMap(), [block|file]SD.getAllVolumes
......................................................................


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

(16 inline comments)

....................................................
File vdsm/storage/image.py
Line 460:         tSelfImage = tImgs[0]
pyflakes image.py
image.py:460: undefined name 'tImgs'
image.py:461: undefined name 'tImgs'

....................................................
File vdsm/storage/sp.py
Line 1717:         # 4) if regular image template based remove image
s/0).*4).*/

In addition to removing image, this function also does the following:
If removing a template which has dependent images - creates a fake template
If removing the last image which depends on a fake template - removes the fake template as well

This should be in the docstring and not here.

Line 1718:         # May be worth block file differentiation?
please remove this comment

Line 1719:         allVols = sdDom.getAllVolumes()
why not pass image to filter by ?

Line 1721:         volsImgs = dict((k, v) for k, v in allVols.iteritems() if imgUUID in v[0])
s/volsImgs/imagesByVol/ ?

Line 1722:         if all(len(v[0]) == 1 for k, v in volsImgs.iteritems()):
Add comment:
Image has no dependent images and does not depend on other images so safe to delete

s/k/vol/
s/v[0]/v.images/
why does volImgs include the parent volume and not just the images?

Line 1723:             # This is a self contained regular image, e.g. no template based
Entire comment is redundant

Line 1728:             # TODO: image.delete should be a sdDom function
remove comment

Line 1730:             # Deleting a template with dependencies or a template based image.
s/#.*/
This is either a template with derived images or a derived image so needs further scrutiny

Line 1733:             # Template for an image should be only one!
Remove comment, log message should be self explanatory.

Line 1735:                 raise se.ImageValidationError("mismatched qty of templates"
Image points to multiple templates, cannot safely delete it.

Line 1738:             # TODO: Lock the template, reload allVols.
just add the lock

Line 1742:             tImgs = template[1][0]
s/template...
vol,((images),parent) = ts[0]
templateImage = images[0]

Line 1744:             tSelfImage = tImgs[0]
remove

Line 1746:             if  numOfDependentImgs < 1:
remove this "if", this will always be false

Line 1764:                 # Fake templates are unknown by the manager, then it's real
remove last 2 lines of comment

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I7eccf5ca100bd354aa09208ca60bb112fb697063
Gerrit-PatchSet: 5
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Eduardo <ewarszaw at redhat.com>
Gerrit-Reviewer: Ayal Baron <abaron at redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken at redhat.com>
Gerrit-Reviewer: Eduardo <ewarszaw at redhat.com>
Gerrit-Reviewer: Igor Lvovsky <ilvovsky at redhat.com>
Gerrit-Reviewer: Saggi Mizrahi <smizrahi at redhat.com>


More information about the vdsm-patches mailing list