Change in vdsm[master]: API: Separate clientIF and the xmlrpc server

Adam Litke has uploaded a new change for review.

Change subject: API: Separate clientIF and the xmlrpc server
......................................................................

API: Separate clientIF and the xmlrpc server

Currently the xmlrpc server functionality is entwined with the clientIF class.
In order to proceed with the API bridge plan (http://ovirt.org/wiki/Vdsm_API) I
would like to separate the xmlrpc server into its own class.  This brings
clientIF one step closer to becoming a generic API Bridge that is able to host
additional bindings such as REST and QMF.

Despite the size of this patch, this was a pretty simple refactoring.  I made
the following choices as I split this out:

- The xmlrpc server binding class has start() and prepareForShutdown() APIs
- clientIF will still own the storage dispatcher but API wrapper mechanisms
  have moved into the xmlrpc class.

Change-Id: I5026e11f3d39343c957b59606e90c23acbb598ba
Signed-off-by: Adam Litke <agl(a)us.ibm.com&gt;
---
M vdsm/clientIF.py
1 file changed, 137 insertions(+), 110 deletions(-)


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

Gerrit-MessageType: newchange
Gerrit-Change-Id: I5026e11f3d39343c957b59606e90c23acbb598ba
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Adam Litke <agl(a)us.ibm.com&gt;

[Dan Kenigsberg]

Dan Kenigsberg has submitted this change and it was merged.

Change subject: API: Separate clientIF and the xmlrpc server
......................................................................


API: Separate clientIF and the xmlrpc server

Currently the xmlrpc server functionality is entwined with the clientIF class.
In order to proceed with the API bridge plan (http://ovirt.org/wiki/Vdsm_API) I
would like to separate the xmlrpc server into its own class.  This brings
clientIF one step closer to becoming a generic API Bridge that is able to host
additional bindings such as REST and QMF.

Despite the size of this patch, this was a pretty simple refactoring.  I made
the following choices as I split this out:

- The xmlrpc server binding class has start() and prepareForShutdown() APIs
- clientIF will still own the storage dispatcher but API wrapper mechanisms
  have moved into the xmlrpc class.

Change-Id: I5026e11f3d39343c957b59606e90c23acbb598ba
Signed-off-by: Adam Litke <agl(a)us.ibm.com&gt;
---
M vdsm/clientIF.py
M vdsm/vm.py
2 files changed, 167 insertions(+), 112 deletions(-)

Approvals:
  Saggi Mizrahi: Verified; Looks good to me, but someone else must approve
  Dan Kenigsberg: Looks good to me, approved


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

Gerrit-MessageType: merged
Gerrit-Change-Id: I5026e11f3d39343c957b59606e90c23acbb598ba
Gerrit-PatchSet: 4
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Adam Litke <agl(a)us.ibm.com&gt;
Gerrit-Reviewer: Adam Litke <agl(a)us.ibm.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Igor Lvovsky <ilvovsky(a)redhat.com&gt;
Gerrit-Reviewer: Saggi Mizrahi <smizrahi(a)redhat.com&gt;

[Dan Kenigsberg]

Dan Kenigsberg has posted comments on this change.

Change subject: API: Separate clientIF and the xmlrpc server
......................................................................


Patch Set 4: Looks good to me, approved

Thanks, Adam.

I'm taking this even though you forgot to update AUTHORS!

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I5026e11f3d39343c957b59606e90c23acbb598ba
Gerrit-PatchSet: 4
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Adam Litke <agl(a)us.ibm.com&gt;
Gerrit-Reviewer: Adam Litke <agl(a)us.ibm.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Igor Lvovsky <ilvovsky(a)redhat.com&gt;
Gerrit-Reviewer: Saggi Mizrahi <smizrahi(a)redhat.com&gt;

Saggi Mizrahi has posted comments on this change.

Change subject: API: Separate clientIF and the xmlrpc server
......................................................................


Patch Set 4: Verified

Ran some verbs and it all seems to work

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I5026e11f3d39343c957b59606e90c23acbb598ba
Gerrit-PatchSet: 4
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Adam Litke <agl(a)us.ibm.com&gt;
Gerrit-Reviewer: Adam Litke <agl(a)us.ibm.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Igor Lvovsky <ilvovsky(a)redhat.com&gt;
Gerrit-Reviewer: Saggi Mizrahi <smizrahi(a)redhat.com&gt;

Saggi Mizrahi has posted comments on this change.

Change subject: API: Separate clientIF and the xmlrpc server
......................................................................


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

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I5026e11f3d39343c957b59606e90c23acbb598ba
Gerrit-PatchSet: 4
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Adam Litke <agl(a)us.ibm.com&gt;
Gerrit-Reviewer: Adam Litke <agl(a)us.ibm.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Igor Lvovsky <ilvovsky(a)redhat.com&gt;
Gerrit-Reviewer: Saggi Mizrahi <smizrahi(a)redhat.com&gt;

[Igor Lvovsky]

Igor Lvovsky has posted comments on this change.

Change subject: API: Separate clientIF and the xmlrpc server
......................................................................


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

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I5026e11f3d39343c957b59606e90c23acbb598ba
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Adam Litke <agl(a)us.ibm.com&gt;
Gerrit-Reviewer: Adam Litke <agl(a)us.ibm.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Igor Lvovsky <ilvovsky(a)redhat.com&gt;
Gerrit-Reviewer: Saggi Mizrahi <smizrahi(a)redhat.com&gt;

Adam Litke has posted comments on this change.

Change subject: API: Separate clientIF and the xmlrpc server
......................................................................


Patch Set 2: (4 inline comments)

....................................................
File vdsm/clientIF.py
Line 62: class bindingXMLRPC:
Done

Line 66:         self.serverPort = config.get('addresses',
'management_port')
Done

Line 77:         self.server.timeout = 1
I am not sure either.  It might not be important to expose this option to the users. 
Either way, could we postpone that for a future series to avoid mixing too many unrelated
changes into the API refactoring?

Line 343:     def getGlobalMethods(self):
Yes, I kept that intact for the HSM methods, but for the methods that are in clientIF, I
did not see a benefit in adding all of that glue since only the xmlrpc binding needs it. 
In a later patch, this will be moved into the API class anyhow.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I5026e11f3d39343c957b59606e90c23acbb598ba
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Adam Litke <agl(a)us.ibm.com&gt;
Gerrit-Reviewer: Adam Litke <agl(a)us.ibm.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Igor Lvovsky <ilvovsky(a)redhat.com&gt;
Gerrit-Reviewer: Saggi Mizrahi <smizrahi(a)redhat.com&gt;

Saggi Mizrahi has posted comments on this change.

Change subject: API: Separate clientIF and the xmlrpc server
......................................................................


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

(4 inline comments)

Looks good, only minor comments.

....................................................
File vdsm/clientIF.py
Line 62: class bindingXMLRPC:
Classes should start with a capital letter.
Please make it a new style class

Line 66:         self.serverPort = config.get('addresses',
'management_port')
Classes other then HSM\ClientIF should not access the configuration unless unavoidable.
Please access the config in client IF and then pass it to the ctor of this calls.

Avoiding touching the config makes unit testing stub creation and a bunch of other stuff
easier.

Line 77:         self.server.timeout = 1
This looks like something that might need to be configurable but I'm not sure about
that.

Line 343:     def getGlobalMethods(self):
You can inspect the hsm object for exported methods.
Look at dispatcher.py on how to do that.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I5026e11f3d39343c957b59606e90c23acbb598ba
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Adam Litke <agl(a)us.ibm.com&gt;
Gerrit-Reviewer: Adam Litke <agl(a)us.ibm.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Igor Lvovsky <ilvovsky(a)redhat.com&gt;
Gerrit-Reviewer: Saggi Mizrahi <smizrahi(a)redhat.com&gt;

[Dan Kenigsberg]

Dan Kenigsberg has posted comments on this change.

Change subject: API: Separate clientIF and the xmlrpc server
......................................................................


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

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I5026e11f3d39343c957b59606e90c23acbb598ba
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Adam Litke <agl(a)us.ibm.com&gt;
Gerrit-Reviewer: Adam Litke <agl(a)us.ibm.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;

[Igor Lvovsky]

Igor Lvovsky has posted comments on this change.

Change subject: API: Separate clientIF and the xmlrpc server
......................................................................


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

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I5026e11f3d39343c957b59606e90c23acbb598ba
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Adam Litke <agl(a)us.ibm.com&gt;
Gerrit-Reviewer: Adam Litke <agl(a)us.ibm.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Igor Lvovsky <ilvovsky(a)redhat.com&gt;

[Dan Kenigsberg]

Dan Kenigsberg has posted comments on this change.

Change subject: API: Separate clientIF and the xmlrpc server
......................................................................


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

Ok, it would look good to me. I'm waiting for a fix patch, to verify it's
harmless, and move forward.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I5026e11f3d39343c957b59606e90c23acbb598ba
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Adam Litke <agl(a)us.ibm.com&gt;
Gerrit-Reviewer: Adam Litke <agl(a)us.ibm.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;

Adam Litke has posted comments on this change.

Change subject: API: Separate clientIF and the xmlrpc server
......................................................................


Patch Set 1: (2 inline comments)

....................................................
File vdsm/clientIF.py
Line 131:                 file(constants.P_VDSM_CLIENT_LOG, 'w')
Done.  Added a FIXME here.

Line 373:                ]
Done

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I5026e11f3d39343c957b59606e90c23acbb598ba
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Adam Litke <agl(a)us.ibm.com&gt;
Gerrit-Reviewer: Adam Litke <agl(a)us.ibm.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;

[Dan Kenigsberg]

Dan Kenigsberg has posted comments on this change.

Change subject: API: Separate clientIF and the xmlrpc server
......................................................................


Patch Set 1: (2 inline comments)

....................................................
File vdsm/clientIF.py
Line 131:                 file(constants.P_VDSM_CLIENT_LOG, 'w')
editNetwork changes host network configuration and waits. If ANOTHER api call (ping,
getVdsCaps, whatever) arrives, we know that we have a working network config, and commit
it.

We use a file, instead of an in-process event, since editNetork is executed in another
process, as root.

It would have been WAY more nicer to `touch` P_VDSM_CLIENT_LOG in an API wrapper (but to
make sure it is not called if vdsm calls its own api from an async task).

We could add a FIXME about it in this patch (to keep up with is advertisement), and solve
it somehow in a later patch.

Line 373:                ]
]->) as this is not expected to change during runtime ;-)

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I5026e11f3d39343c957b59606e90c23acbb598ba
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Adam Litke <agl(a)us.ibm.com&gt;
Gerrit-Reviewer: Adam Litke <agl(a)us.ibm.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;

Adam Litke has posted comments on this change.

Change subject: API: Separate clientIF and the xmlrpc server
......................................................................


Patch Set 1: (3 inline comments)

....................................................
File vdsm/clientIF.py
Line 131:                 file(constants.P_VDSM_CLIENT_LOG, 'w')
Hmm, I don't quite understand.  How would touching a local log file serve to verify
whether a network connection has been severed?

Assuming this remains the best way to accomplish this check, would it be acceptable to
just move the log file touch into the editNetwork verb itself?

Line 173:         
Done

Line 290:             # stop listening ASAP
Done

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I5026e11f3d39343c957b59606e90c23acbb598ba
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Adam Litke <agl(a)us.ibm.com&gt;
Gerrit-Reviewer: Adam Litke <agl(a)us.ibm.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;

[Dan Kenigsberg]

Dan Kenigsberg has posted comments on this change.

Change subject: API: Separate clientIF and the xmlrpc server
......................................................................


Patch Set 1: (3 inline comments)

Thanks Adam, I like the direction.

(no time for complete review now)

....................................................
File vdsm/clientIF.py
Line 131:                 file(constants.P_VDSM_CLIENT_LOG, 'w')
ouch, the original code is painfully intertwining the xmlrpc transport with
functionality.

this touching of the P_VDSM_CLIENT_LOG file, is required by editNetwork in order to tell
that it did not cut its line of communication.

this probably has to be fixed in another patch.

Line 173:         
painfully red

Line 290:             # stop listening ASAP
this comment has just rotten...

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I5026e11f3d39343c957b59606e90c23acbb598ba
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Adam Litke <agl(a)us.ibm.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;