Change in vdsm[master]: clientIF: Teardown volume path only for VDSM images
sgotliv at redhat.com
sgotliv at redhat.com
Fri Dec 13 15:21:44 UTC 2013
Sergey Gotliv has posted comments on this change.
Change subject: clientIF: Teardown volume path only for VDSM images
......................................................................
Patch Set 5:
(1 comment)
@Nir, you introduced a nice patch, but please see Ayal's comment "device == disk" should be removed from "prepare" so how it will help me and that patch if you put it in the place I can't remove it :-)?
....................................................
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. Federico the check will be symmetrical after I'll remove the check that device is a disk in "prepare" in another patch. Please agree with me that this redundant check shouldn't be added in the newly written code just because of symmetry - all we need to check for "prepare" and "teardown" is a PDIV.
2. Before my change "teardown" didn't care about "device == 'disk' and I don't want to add it now and remove it in the next page. If you trusted this method before (prepare with checking device - teardown without) you should continue to trust it
3. The reason why I didn't do that till now is actually reviewers - practically everything is going too hard - you remove things in the one place and they want it all over the code (see Nir's and Michal's comments in this patch - VM contains a lot of these checkings) - this all or nothing approach is very time consuming.
4. If cdrom one day will be a PDIV we'll need to review the entire flow for that, because currently "prepare" doesn't return the cdrom path as a PDIV so in the future "prepare" and "teardown" will be reviewed in the context of the cdrom.
Line 330: res = self.irs.teardownImage(drive['domainID'],
Line 331: drive['poolID'], drive['imageID'])
Line 332: return res['status']['code']
Line 333:
--
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 at redhat.com>
Gerrit-Reviewer: Allon Mureinik <amureini at redhat.com>
Gerrit-Reviewer: Ayal Baron <abaron at redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken at redhat.com>
Gerrit-Reviewer: Federico Simoncelli <fsimonce at redhat.com>
Gerrit-Reviewer: Francesco Romani <fromani at redhat.com>
Gerrit-Reviewer: Michal Skrivanek <michal.skrivanek at redhat.com>
Gerrit-Reviewer: Nir Soffer <nsoffer at redhat.com>
Gerrit-Reviewer: Sergey Gotliv <sgotliv at redhat.com>
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes
More information about the vdsm-patches
mailing list