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