Adam Litke has uploaded a new change for review.
Change subject: Add an object model to clientIF ......................................................................
Add an object model to clientIF
Create an object-based API for vdsm. The current objects are: Task, StoragePool, StorageDomain, Image, Volume, ISCSIConnection, LVMVolumeGroup, and VM. Additionally, a Global object currently handles APIs which don't map nicely to one of the above objects. Right now this interface is only an abstraction that maps directly to the flat function namespace. This will change later in the patch series and more over time. Since this API is internal (it only affects bindings), we can continue to change it over time without affecting ovirt-engine.
Change-Id: I2ee54277c87a02ac8dafe29a8761e2acf0f7397f Signed-off-by: Adam Litke agl@us.ibm.com --- M vdsm.spec.in A vdsm/API.py M vdsm/Makefile.am 3 files changed, 480 insertions(+), 0 deletions(-)
git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/84/984/1 -- To view, visit http://gerrit.ovirt.org/984 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: newchange Gerrit-Change-Id: I2ee54277c87a02ac8dafe29a8761e2acf0f7397f Gerrit-PatchSet: 1 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Adam Litke agl@us.ibm.com
Adam Litke has posted comments on this change.
Change subject: Add an object model to clientIF ......................................................................
Patch Set 1: (7 inline comments)
Ewoud, thank you for your review! I have corrected the whitespace issues and have countered with my opinions on dict.copy() in this particular case.
.................................................... File vdsm/API.py Line 27: class VM(object): Done
Line 43: return self._cif.create(createParameters) Hmm, I understand why you would suggest making a copy of the dict here but there are reasons why I would suggest keeping it the way it is:
1) createParams must contain a key 'vmId' whose value matches the UUID of the currently instantiated vm object. Therefore, this modification of the dictionary serves to enforce a rule, not override user intention.
2) This dict contains other structures and is quite large. Therefore an inefficient deepcopy() would be required. Besides, the actual create method already makes alterations to the params dict.
Line 72: return self._cif.migrate(params) I would propose the same reasons here for keeping it the way it is.
Line 78: params['vmId'] = self._UUID ... and here.
Line 373: Done
Line 406: # This internal API deviates horribly in that it takes a Done
Line 408: _vmString = string.join(vmList, ',') Done
-- To view, visit http://gerrit.ovirt.org/984 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: I2ee54277c87a02ac8dafe29a8761e2acf0f7397f Gerrit-PatchSet: 1 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Adam Litke agl@us.ibm.com Gerrit-Reviewer: Adam Litke agl@us.ibm.com Gerrit-Reviewer: Ewoud Kohl van Wijngaarden ewoud@kohlvanwijngaarden.nl Gerrit-Reviewer: Saggi Mizrahi smizrahi@redhat.com
Ewoud Kohl van Wijngaarden has posted comments on this change.
Change subject: Add an object model to clientIF ......................................................................
Patch Set 1: (7 inline comments)
Relative small suggestions.
.................................................... File vdsm/API.py Line 27: class VM(object): Please remove the trailing whitespace
Line 43: return self._cif.create(createParameters) I wouldn't modify the original dict, but work on a copy obtained by dict.copy().
Line 72: return self._cif.migrate(params) Same here about dict.copy()
Line 78: params['vmId'] = self._UUID Same here about dict.copy()
Line 373: Please remove the trailing whitespace
Line 406: # This internal API deviates horribly in that it takes a Please remove the trailing whitespace
Line 408: _vmString = string.join(vmList, ',') string.join() is deprecated. Please use ','.join(vmList).
-- To view, visit http://gerrit.ovirt.org/984 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: I2ee54277c87a02ac8dafe29a8761e2acf0f7397f Gerrit-PatchSet: 1 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Adam Litke agl@us.ibm.com Gerrit-Reviewer: Ewoud Kohl van Wijngaarden ewoud@kohlvanwijngaarden.nl
Ewoud Kohl van Wijngaarden has posted comments on this change.
Change subject: Add an object model to clientIF ......................................................................
Patch Set 1: (1 inline comment)
I think you have a valid point.
.................................................... File vdsm/API.py Line 43: return self._cif.create(createParameters) This does create side effects because it modifies the original dict. By working on a shallow copy would ensure there are no side effects to this particular function, but if the called function doesn't respect that I don't have a particular strong opinion about this. It may be better to solve this properly (or not at all) than partially.
-- To view, visit http://gerrit.ovirt.org/984 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: I2ee54277c87a02ac8dafe29a8761e2acf0f7397f Gerrit-PatchSet: 1 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Adam Litke agl@us.ibm.com Gerrit-Reviewer: Adam Litke agl@us.ibm.com Gerrit-Reviewer: Ewoud Kohl van Wijngaarden ewoud@kohlvanwijngaarden.nl Gerrit-Reviewer: Saggi Mizrahi smizrahi@redhat.com
Dan Kenigsberg has posted comments on this change.
Change subject: Add an object model to clientIF ......................................................................
Patch Set 1: (1 inline comment)
.................................................... File vdsm/API.py Line 1: import string missing GPL boiler plate
-- To view, visit http://gerrit.ovirt.org/984 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: I2ee54277c87a02ac8dafe29a8761e2acf0f7397f Gerrit-PatchSet: 1 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Adam Litke agl@us.ibm.com Gerrit-Reviewer: Adam Litke agl@us.ibm.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Ewoud Kohl van Wijngaarden ewoud@kohlvanwijngaarden.nl Gerrit-Reviewer: Saggi Mizrahi smizrahi@redhat.com
Ewoud Kohl van Wijngaarden has posted comments on this change.
Change subject: Add an object model to clientIF ......................................................................
Patch Set 1: (1 inline comment)
.................................................... File vdsm/API.py Line 1: import string When you've changed string.join(vmList, ',') to ','.join(vmList) you can remove this import as well.
-- To view, visit http://gerrit.ovirt.org/984 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: I2ee54277c87a02ac8dafe29a8761e2acf0f7397f Gerrit-PatchSet: 1 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Adam Litke agl@us.ibm.com Gerrit-Reviewer: Adam Litke agl@us.ibm.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Ewoud Kohl van Wijngaarden ewoud@kohlvanwijngaarden.nl Gerrit-Reviewer: Saggi Mizrahi smizrahi@redhat.com
Saggi Mizrahi has posted comments on this change.
Change subject: Add an object model to clientIF ......................................................................
Patch Set 1: Looks good to me, but someone else must approve
There are stuff here that I don't agree with like Volume having an object and some of the API semantics but these should be discussed unrelated to the actual code concept.
I do agree with the general fact that we should have a group f objects mapping to logical units inside VDSM.
API.py I would like to see the module in lower case and maybe have a more indicative name but this can be done later.
-- To view, visit http://gerrit.ovirt.org/984 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: I2ee54277c87a02ac8dafe29a8761e2acf0f7397f Gerrit-PatchSet: 1 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Adam Litke agl@us.ibm.com Gerrit-Reviewer: Adam Litke agl@us.ibm.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Ewoud Kohl van Wijngaarden ewoud@kohlvanwijngaarden.nl Gerrit-Reviewer: Saggi Mizrahi smizrahi@redhat.com
Saggi Mizrahi has posted comments on this change.
Change subject: Add an object model to clientIF ......................................................................
Patch Set 3: Looks good to me, but someone else must approve
(1 inline comment)
I;m ignoring the actual fields exported as they will change as the API for 4.0 gets more and more clear.
.................................................... File vdsm/API.py Line 40: return self._irs.revertTask(self._UUID) Don't export revert, it will not be supported in the future.
-- To view, visit http://gerrit.ovirt.org/984 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: I2ee54277c87a02ac8dafe29a8761e2acf0f7397f Gerrit-PatchSet: 3 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Adam Litke agl@us.ibm.com Gerrit-Reviewer: Adam Litke agl@us.ibm.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Ewoud Kohl van Wijngaarden ewoud@kohlvanwijngaarden.nl Gerrit-Reviewer: Saggi Mizrahi smizrahi@redhat.com
Saggi Mizrahi has posted comments on this change.
Change subject: Add an object model to clientIF ......................................................................
Patch Set 3: Verified
Ran some verbs and it all seems to work
-- To view, visit http://gerrit.ovirt.org/984 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: I2ee54277c87a02ac8dafe29a8761e2acf0f7397f Gerrit-PatchSet: 3 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Adam Litke agl@us.ibm.com Gerrit-Reviewer: Adam Litke agl@us.ibm.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Ewoud Kohl van Wijngaarden ewoud@kohlvanwijngaarden.nl Gerrit-Reviewer: Saggi Mizrahi smizrahi@redhat.com
Dan Kenigsberg has posted comments on this change.
Change subject: Add an object model to clientIF ......................................................................
Patch Set 4: Verified; Looks good to me, approved
-- To view, visit http://gerrit.ovirt.org/984 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: I2ee54277c87a02ac8dafe29a8761e2acf0f7397f Gerrit-PatchSet: 4 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Adam Litke agl@us.ibm.com Gerrit-Reviewer: Adam Litke agl@us.ibm.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Ewoud Kohl van Wijngaarden ewoud@kohlvanwijngaarden.nl Gerrit-Reviewer: Saggi Mizrahi smizrahi@redhat.com
Dan Kenigsberg has submitted this change and it was merged.
Change subject: Add an object model to clientIF ......................................................................
Add an object model to clientIF
Create an object-based API for vdsm. The current objects are: Task, StoragePool, StorageDomain, Image, Volume, ISCSIConnection, LVMVolumeGroup, and VM. Additionally, a Global object currently handles APIs which don't map nicely to one of the above objects. Right now this interface is only an abstraction that maps directly to the flat function namespace. This will change later in the patch series and more over time. Since this API is internal (it only affects bindings), we can continue to change it over time without affecting ovirt-engine.
Change-Id: I2ee54277c87a02ac8dafe29a8761e2acf0f7397f Signed-off-by: Adam Litke agl@us.ibm.com --- M vdsm.spec.in A vdsm/API.py M vdsm/Makefile.am 3 files changed, 509 insertions(+), 0 deletions(-)
Approvals: Dan Kenigsberg: Verified; Looks good to me, approved
-- To view, visit http://gerrit.ovirt.org/984 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: merged Gerrit-Change-Id: I2ee54277c87a02ac8dafe29a8761e2acf0f7397f Gerrit-PatchSet: 4 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Adam Litke agl@us.ibm.com Gerrit-Reviewer: Adam Litke agl@us.ibm.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Ewoud Kohl van Wijngaarden ewoud@kohlvanwijngaarden.nl Gerrit-Reviewer: Saggi Mizrahi smizrahi@redhat.com
vdsm-patches@lists.fedorahosted.org