Change in vdsm[master]: Move API out of clientIF

Adam Litke has uploaded a new change for review.

Change subject: Move API out of clientIF
......................................................................

Move API out of clientIF

Many API functions are implemented inside clientIF but this is no longer the
proper place for them.  Move API-related functions into their proper objects
inside API.py.  clientIF still owns the storage dispatcher and the vm list.  In
the future, I would like to move these out of clientIF as well but I have
decided to reserve that for a future patch series.

Change-Id: I94ad036cfaf3d1bec2f5b4049979a1bf0e2810ce
Signed-off-by: Adam Litke 
---
M vdsm/API.py
M vdsm/bindingXMLRPC.py
M vdsm/clientIF.py
3 files changed, 1,347 insertions(+), 897 deletions(-)

git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/85/985/1
--
To view, visit http://gerrit.ovirt.org/985
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: newchange
Gerrit-Change-Id: I94ad036cfaf3d1bec2f5b4049979a1bf0e2810ce
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Adam Litke

Adam Litke has posted comments on this change.

Change subject: Move API out of clientIF
......................................................................

Patch Set 1: (6 inline comments)

Ewoud, thank you for your review and comments.  Whitespace issues have been fixed.

....................................................
File vdsm/API.py
Line 267:         self._cif.vmContainerLock.acquire()
Good suggestion, but this patch is just moving the implementation from one place to another.  I would prefer not to make functional changes to the code at the same time.

Line 300:         return {'status': doneCode, 'statsList': [stats]}            
Done

Line 470:     
Done

Line 1192:                 options[_translationMap[k]] = options.pop(k)        
Done

....................................................
File vdsm/bindingXMLRPC.py
Line 312:         
Done

Line 465:            
Done

--
To view, visit http://gerrit.ovirt.org/985
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I94ad036cfaf3d1bec2f5b4049979a1bf0e2810ce
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Adam Litke 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Ewoud Kohl van Wijngaarden

Ewoud Kohl van Wijngaarden has posted comments on this change.

Change subject: Move API out of clientIF
......................................................................

Patch Set 1: (6 inline comments)

Mostly whitespace related.

I only once suggested to use with self._cif_vmContainerLock, but that could be used more often.

....................................................
File vdsm/API.py
Line 267:         self._cif.vmContainerLock.acquire()
Perhaps you could use the with statement:

with self._cif.vmContainerLock:
    self.log.info("vmContainerLock acquired by vm %s", self._UUID)
    v = self._cif.vmContainer.get(self._UUID)

It's the same as using the try-finally-release, unless self.log.info throws an exception. Also easier on the eyes imho.

Line 300:         return {'status': doneCode, 'statsList': [stats]}            
Please remove the trailing whitespace

Line 470:     
Please remove the trailing whitespace

Line 1192:                 options[_translationMap[k]] = options.pop(k)        
Please remove the trailing whitespace

....................................................
File vdsm/bindingXMLRPC.py
Line 312:         
Please remove trailing whitespace

Line 465:            
Please remove trailing whitespace

--
To view, visit http://gerrit.ovirt.org/985
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I94ad036cfaf3d1bec2f5b4049979a1bf0e2810ce
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Adam Litke 
Gerrit-Reviewer: Ewoud Kohl van Wijngaarden

Ewoud Kohl van Wijngaarden has posted comments on this change.

Change subject: Move API out of clientIF
......................................................................

Patch Set 1: (1 inline comment)

Again a valid point.

....................................................
File vdsm/API.py
Line 267:         self._cif.vmContainerLock.acquire()
Very true.

--
To view, visit http://gerrit.ovirt.org/985
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I94ad036cfaf3d1bec2f5b4049979a1bf0e2810ce
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Adam Litke 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Ewoud Kohl van Wijngaarden

[Dan Kenigsberg]

Dan Kenigsberg has posted comments on this change.

Change subject: Move API out of clientIF
......................................................................

Patch Set 1: (1 inline comment)

....................................................
File vdsm/bindingXMLRPC.py
Line 297:     def domainActivate(self, sdUUID, spUUID, options = None):
Could you keep arg=default without spaces, and keep lines shorter than 80chars.

--
To view, visit http://gerrit.ovirt.org/985
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I94ad036cfaf3d1bec2f5b4049979a1bf0e2810ce
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Adam Litke 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Ewoud Kohl van Wijngaarden

Saggi Mizrahi has posted comments on this change.

Change subject: Move API out of clientIF
......................................................................

Patch Set 1: I would prefer that you didn't submit this

As I stressed in the VDSM sync call I don't think all the API calls should be registered in clientIF but rather have a mechanism for the subsystems to declare exported methods (we already have code in hsm\dispatcher) and the binding wrapper should introspect the subsystems.

--
To view, visit http://gerrit.ovirt.org/985
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I94ad036cfaf3d1bec2f5b4049979a1bf0e2810ce
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Adam Litke 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Ewoud Kohl van Wijngaarden 
Gerrit-Reviewer: Saggi Mizrahi

Adam Litke has posted comments on this change.

Change subject: Move API out of clientIF
......................................................................

Patch Set 1:

Saggi, the major downsides to allowing subsystems to register exported functions are that: a) it becomes difficult to gather a comprehensive list of exported functions because they are scattered throughout the code, and b) exported functions need to be organized into the object model so (with the exception of xmlrpc bindings) automatically-generated lists of functions will not be needed.

--
To view, visit http://gerrit.ovirt.org/985
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I94ad036cfaf3d1bec2f5b4049979a1bf0e2810ce
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Adam Litke 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Ewoud Kohl van Wijngaarden 
Gerrit-Reviewer: Saggi Mizrahi

Saggi Mizrahi has posted comments on this change.

Change subject: Move API out of clientIF
......................................................................

Patch Set 3: Looks good to me, but someone else must approve

--
To view, visit http://gerrit.ovirt.org/985
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I94ad036cfaf3d1bec2f5b4049979a1bf0e2810ce
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Adam Litke 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Ewoud Kohl van Wijngaarden 
Gerrit-Reviewer: Saggi Mizrahi

Saggi Mizrahi has posted comments on this change.

Change subject: Move API out of clientIF
......................................................................

Patch Set 3: Verified

Ran some verbs and it all seems to work

--
To view, visit http://gerrit.ovirt.org/985
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I94ad036cfaf3d1bec2f5b4049979a1bf0e2810ce
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Adam Litke 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Ewoud Kohl van Wijngaarden 
Gerrit-Reviewer: Saggi Mizrahi

[Dan Kenigsberg]

Dan Kenigsberg has posted comments on this change.

Change subject: Move API out of clientIF
......................................................................

Patch Set 4: Verified; Looks good to me, approved

--
To view, visit http://gerrit.ovirt.org/985
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I94ad036cfaf3d1bec2f5b4049979a1bf0e2810ce
Gerrit-PatchSet: 4
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Adam Litke 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Ewoud Kohl van Wijngaarden 
Gerrit-Reviewer: Saggi Mizrahi

[Dan Kenigsberg]

Dan Kenigsberg has submitted this change and it was merged.

Change subject: Move API out of clientIF
......................................................................

Move API out of clientIF

Many API functions are implemented inside clientIF but this is no longer the
proper place for them.  Move API-related functions into their proper objects
inside API.py.  clientIF still owns the storage dispatcher and the vm list.  In
the future, I would like to move these out of clientIF as well but I have
decided to reserve that for a future patch series.

Change-Id: I94ad036cfaf3d1bec2f5b4049979a1bf0e2810ce
Signed-off-by: Adam Litke 
---
M vdsm/API.py
M vdsm/BindingXMLRPC.py
M vdsm/clientIF.py
3 files changed, 1,409 insertions(+), 944 deletions(-)

Approvals:
  Dan Kenigsberg: Verified; Looks good to me, approved

--
To view, visit http://gerrit.ovirt.org/985
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: merged
Gerrit-Change-Id: I94ad036cfaf3d1bec2f5b4049979a1bf0e2810ce
Gerrit-PatchSet: 4
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Adam Litke 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Ewoud Kohl van Wijngaarden 
Gerrit-Reviewer: Saggi Mizrahi