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 <[email protected]>
Gerrit-Reviewer: Allon Mureinik <[email protected]>
Gerrit-Reviewer: Ayal Baron <[email protected]>
Gerrit-Reviewer: Dan Kenigsberg <[email protected]>
Gerrit-Reviewer: Federico Simoncelli <[email protected]>
Gerrit-Reviewer: Francesco Romani <[email protected]>
Gerrit-Reviewer: Michal Skrivanek <[email protected]>
Gerrit-Reviewer: Nir Soffer <[email protected]>
Gerrit-Reviewer: Sergey Gotliv <[email protected]>
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes
_______________________________________________
vdsm-patches mailing list
[email protected]
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
