Federico Simoncelli has posted comments on this change.
Change subject: clientIF: Teardown volume path only for VDSM images
......................................................................
Patch Set 5:
(3 comments)
....................................................
File vdsm/clientIF.py
Line 325:
Line 326: def teardownVolumePath(self, drive):
Line 327: res = {'status': doneCode}
Line 328:
Line 329: if vm.isVdsmImage(drive):
+1, this is what I was referring to as "symmetrical" in one of my previous
comments.
Line 330: res = self.irs.teardownImage(drive['domainID'],
Line 331: drive['poolID'],
drive['imageID'])
Line 332: return res['status']['code']
Line 333:
Line 326: def teardownVolumePath(self, drive):
Line 327: res = {'status': doneCode}
Line 328:
Line 329: if vm.isVdsmImage(drive):
Line 330: res = self.irs.teardownImage(drive['domainID'],
I am fine with the exception block being removed, but you have to consider that now any
code that loops over drives:
for drive in drivesList:
drive.teardownVolumePath(...)
can now fail at any point and prevent the remaining teardowns. Please make sure that this
never happens.
Line 331: drive['poolID'],
drive['imageID'])
Line 332: return res['status']['code']
Line 333:
Line 334: def getDiskAlignment(self, drive):
....................................................
File vdsm/vm.py
Line 81:
Line 82:
Line 83: def isVdsmImage(drive):
Line 84: if hasattr(drive, "isVdsmImage"):
Line 85: return drive.isVdsmImage()
My advice is to keep just one method/function. I'm in favor of removing
Drive.isVdsmImage, and keep this one only (capable of dealing with both drive objects and
dictionaries). In general I would despise this suggestion but I sadly it seems the way to
go to avoid code duplication and have it all implemented in one place. If you have any
design that is avoiding the checks being duplicated in different places, please go ahead
with that.
For example, in a perfect world you would call isVdsmImage(dictionary) when you have a
dictionary and drive.isVdsmImage when you have a drive object. Now since
teardownVolumePath accepts both it might make sense to move this hasattr(drive,
"isVdsmImage") check there.
Line 86: elif type(drive) is dict:
Line 87: return all(k in drive.keys() for k in ('volumeID',
'domainID',
Line 88: 'imageID',
'poolID'))
Line 89: else:
--
To view, visit
http://gerrit.ovirt.org/21973
To unsubscribe, visit
http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I041a306636c75a7aa37d4d7c0811366d80fe609c
Gerrit-PatchSet: 5
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Sergey Gotliv <sgotliv(a)redhat.com>
Gerrit-Reviewer: Allon Mureinik <amureini(a)redhat.com>
Gerrit-Reviewer: Ayal Baron <abaron(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Federico Simoncelli <fsimonce(a)redhat.com>
Gerrit-Reviewer: Michal Skrivanek <michal.skrivanek(a)redhat.com>
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com>
Gerrit-Reviewer: Sergey Gotliv <sgotliv(a)redhat.com>
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes