Change in vdsm[master]: concurrency: Vdscli uses a new HTTP conn per req.

Antoni Segura Puimedon has uploaded a new change for review.

Change subject: concurrency: Vdscli uses a new HTTP conn per req.
......................................................................

concurrency: Vdscli uses a new HTTP conn per req.

Python-2.7 changed xmlrpclib's implementation detail of spawining
a new http/https connection per each request to add support for
Keepalive. Unfortunately, this implementation detail was also what
made the xmlrpclib's Transport be thread-safe.

This change reverts the change in Transport, thus mantaining the
thread-safety of our python client code for unencrypted
communications.

Change-Id: Ifb08df759051bb340423958280e88df5792e9ed2
Signed-off-by: Antoni S. Puimedon 
---
M vdsm_cli/vdscli.py.in
1 file changed, 21 insertions(+), 4 deletions(-)

git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/29/9329/1

diff --git a/vdsm_cli/vdscli.py.in b/vdsm_cli/vdscli.py.in
index 2af41b7..9d5d8a3 100644
--- a/vdsm_cli/vdscli.py.in
+++ b/vdsm_cli/vdscli.py.in
@@ -19,7 +19,8 @@
 # Refer to the README and COPYING files for full details of the license
 #

-import xmlrpclib
+from xmlrpclib import Server, ServerProxy, Transport
+import httplib
 import subprocess
 import os
 from vdsm import SecureXMLRPCServer
@@ -28,6 +29,21 @@
 d_tsPath = '@TRUSTSTORE@'
 d_addr = '0'
 d_port = '54321'
+
+
+class SingleRequestTransport(Transport):
+    '''Python 2.7 Transport introduced a change that makes it reuse connections
+    by default when new connections are requested for a host with an existing
+    connection. This class reverts the change to avoid the concurrency
+    issues.'''
+
+    def make_connection(self, host):
+        '''Creates a new HTTPConnection to the host.'''
+        # create a HTTP connection object from a host descriptor
+        chost, self._extra_headers, x509 = self.get_host_info(host)
+        #store the host argument along with the connection object
+        self._connection = host, httplib.HTTPConnection(chost)
+        return self._connection[1]

def __guessDefaults():
@@ -109,10 +125,11 @@

transport = TransportClass(key_file=KEYFILE,
                                    cert_file=CERTFILE, ca_certs=CACERT)
-        server = xmlrpclib.ServerProxy('https://%s' % addrport,
-                                       transport=transport)
+        server = ServerProxy('https://%s' % addrport,
+                             transport=transport)
     else:
-        server = xmlrpclib.Server('http://%s' % addrport)
+        server = Server('http://%s' % addrport,
+                        transport=SingleRequestTransport(use_datetime=0))
     return server

if __name__ == '__main__':

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

Gerrit-MessageType: newchange
Gerrit-Change-Id: Ifb08df759051bb340423958280e88df5792e9ed2
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Antoni Segura Puimedon

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: concurrency: Vdscli uses a new HTTP conn per req.
......................................................................

Patch Set 1:

Build Started http://jenkins.ovirt.org/job/vdsm_unit_tests_manual_gerrit/75/ (2/2)

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ifb08df759051bb340423958280e88df5792e9ed2
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Antoni Segura Puimedon 
Gerrit-Reviewer: oVirt Jenkins CI Server

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: concurrency: Vdscli uses a new HTTP conn per req.
......................................................................

Patch Set 1:

Build Started http://jenkins.ovirt.org/job/vdsm_pep8_gerrit/52/ (1/2)

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ifb08df759051bb340423958280e88df5792e9ed2
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Antoni Segura Puimedon 
Gerrit-Reviewer: oVirt Jenkins CI Server

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: concurrency: Vdscli uses a new HTTP conn per req.
......................................................................

Patch Set 1:

Build Successful

http://jenkins.ovirt.org/job/vdsm_unit_tests_manual_gerrit/75/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_pep8_gerrit/52/ : SUCCESS

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ifb08df759051bb340423958280e88df5792e9ed2
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Antoni Segura Puimedon 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Igor Lvovsky 
Gerrit-Reviewer: Livnat Peer 
Gerrit-Reviewer: oVirt Jenkins CI Server

[Dan Kenigsberg]

Dan Kenigsberg has posted comments on this change.

Change subject: concurrency: Vdscli uses a new HTTP conn per req.
......................................................................

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

(4 inline comments)

minor comments, thanks!

....................................................
Commit Message
Line 11: Keepalive. Unfortunately, this implementation detail was also what
Line 12: made the xmlrpclib's Transport be thread-safe.
Line 13: 
Line 14: This change reverts the change in Transport, thus mantaining the
Line 15: thread-safety of our python client code for unencrypted
ah, I was not aware that this problem affects only plaintext. Good. it make the issue even smaller.
Line 16: communications.
Line 17: 
Line 18: Change-Id: Ifb08df759051bb340423958280e88df5792e9ed2

....................................................
File vdsm_cli/vdscli.py.in
Line 18: #
Line 19: # Refer to the README and COPYING files for full details of the license
Line 20: #
Line 21: 
Line 22: from xmlrpclib import Server, ServerProxy, Transport
I was happier when xmlrpclib was mentioned explicitly in code. np, just a matter of taste (and ease of grep'ing).
Line 23: import httplib
Line 24: import subprocess
Line 25: import os
Line 26: from vdsm import SecureXMLRPCServer

Line 36:     by default when new connections are requested for a host with an existing
Line 37:     connection. This class reverts the change to avoid the concurrency
Line 38:     issues.'''
Line 39: 
Line 40:     def make_connection(self, host):
how about avoid code duplication (and possible licensing headache) by setting self._connection = None and calling xmlrcplib.Transport.make_connection()?
Line 41:         '''Creates a new HTTPConnection to the host.'''
Line 42:         # create a HTTP connection object from a host descriptor
Line 43:         chost, self._extra_headers, x509 = self.get_host_info(host)
Line 44:         #store the host argument along with the connection object

Line 128:         server = ServerProxy('https://%s' % addrport,
Line 129:                              transport=transport)
Line 130:     else:
Line 131:         server = Server('http://%s' % addrport,
Line 132:                         transport=SingleRequestTransport(use_datetime=0))
why mention this use_datetime obscure argument?
Line 133:     return server
Line 134: 
Line 135: if __name__ == '__main__':
Line 136:     print 'connecting to %s:%s ssl %s ts %s' % (d_addr, d_port,

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ifb08df759051bb340423958280e88df5792e9ed2
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Antoni Segura Puimedon 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Igor Lvovsky 
Gerrit-Reviewer: Livnat Peer 
Gerrit-Reviewer: oVirt Jenkins CI Server

Antoni Segura Puimedon has posted comments on this change.

Change subject: concurrency: Vdscli uses a new HTTP conn per req.
......................................................................

Patch Set 1: (4 inline comments)

....................................................
Commit Message
Line 11: Keepalive. Unfortunately, this implementation detail was also what
Line 12: made the xmlrpclib's Transport be thread-safe.
Line 13: 
Line 14: This change reverts the change in Transport, thus mantaining the
Line 15: thread-safety of our python client code for unencrypted
Yes. Only plain text connections due to the fact that we inherited and overrode the connection making code of the safe transport in python 2.6 times (when they had the one connection per request implementation detail).
Line 16: communications.
Line 17: 
Line 18: Change-Id: Ifb08df759051bb340423958280e88df5792e9ed2

....................................................
File vdsm_cli/vdscli.py.in
Line 18: #
Line 19: # Refer to the README and COPYING files for full details of the license
Line 20: #
Line 21: 
Line 22: from xmlrpclib import Server, ServerProxy, Transport
Fine. I'll revert it.
Line 23: import httplib
Line 24: import subprocess
Line 25: import os
Line 26: from vdsm import SecureXMLRPCServer

Line 36:     by default when new connections are requested for a host with an existing
Line 37:     connection. This class reverts the change to avoid the concurrency
Line 38:     issues.'''
Line 39: 
Line 40:     def make_connection(self, host):
Ok. I kind of liked to make the change in behavior explicit in the docstring, but since the class name and class doc already hint it, we will be fine.
Line 41:         '''Creates a new HTTPConnection to the host.'''
Line 42:         # create a HTTP connection object from a host descriptor
Line 43:         chost, self._extra_headers, x509 = self.get_host_info(host)
Line 44:         #store the host argument along with the connection object

Line 128:         server = ServerProxy('https://%s' % addrport,
Line 129:                              transport=transport)
Line 130:     else:
Line 131:         server = Server('http://%s' % addrport,
Line 132:                         transport=SingleRequestTransport(use_datetime=0))
Some amount of fail. Fixed.
Line 133:     return server
Line 134: 
Line 135: if __name__ == '__main__':
Line 136:     print 'connecting to %s:%s ssl %s ts %s' % (d_addr, d_port,

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ifb08df759051bb340423958280e88df5792e9ed2
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Antoni Segura Puimedon 
Gerrit-Reviewer: Antoni Segura Puimedon 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Igor Lvovsky 
Gerrit-Reviewer: Livnat Peer 
Gerrit-Reviewer: oVirt Jenkins CI Server

Antoni Segura Puimedon has posted comments on this change.

Change subject: concurrency: Vdscli uses a new HTTP conn per req.
......................................................................

Patch Set 1: (1 inline comment)

....................................................
File vdsm_cli/vdscli.py.in
Line 36:     by default when new connections are requested for a host with an existing
Line 37:     connection. This class reverts the change to avoid the concurrency
Line 38:     issues.'''
Line 39: 
Line 40:     def make_connection(self, host):
Ok, I re-read your comment and now I got the proper meaning :-P. Fixed, then.
Line 41:         '''Creates a new HTTPConnection to the host.'''
Line 42:         # create a HTTP connection object from a host descriptor
Line 43:         chost, self._extra_headers, x509 = self.get_host_info(host)
Line 44:         #store the host argument along with the connection object

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ifb08df759051bb340423958280e88df5792e9ed2
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Antoni Segura Puimedon 
Gerrit-Reviewer: Antoni Segura Puimedon 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Igor Lvovsky 
Gerrit-Reviewer: Livnat Peer 
Gerrit-Reviewer: oVirt Jenkins CI Server

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: concurrency: Vdscli uses a new HTTP conn per req.
......................................................................

Patch Set 2:

Build Started http://jenkins.ovirt.org/job/vdsm_pep8_gerrit/55/ (1/2)

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ifb08df759051bb340423958280e88df5792e9ed2
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Antoni Segura Puimedon 
Gerrit-Reviewer: Antoni Segura Puimedon 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Igor Lvovsky 
Gerrit-Reviewer: Livnat Peer 
Gerrit-Reviewer: oVirt Jenkins CI Server

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: concurrency: Vdscli uses a new HTTP conn per req.
......................................................................

Patch Set 2:

Build Started http://jenkins.ovirt.org/job/vdsm_unit_tests_manual_gerrit/78/ (2/2)

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ifb08df759051bb340423958280e88df5792e9ed2
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Antoni Segura Puimedon 
Gerrit-Reviewer: Antoni Segura Puimedon 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Igor Lvovsky 
Gerrit-Reviewer: Livnat Peer 
Gerrit-Reviewer: oVirt Jenkins CI Server

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: concurrency: Vdscli uses a new HTTP conn per req.
......................................................................

Patch Set 2: Fails

Build Failed

http://jenkins.ovirt.org/job/vdsm_unit_tests_manual_gerrit/78/ : FAILURE

http://jenkins.ovirt.org/job/vdsm_pep8_gerrit/55/ : SUCCESS

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ifb08df759051bb340423958280e88df5792e9ed2
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Antoni Segura Puimedon 
Gerrit-Reviewer: Antoni Segura Puimedon 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Igor Lvovsky 
Gerrit-Reviewer: Livnat Peer 
Gerrit-Reviewer: oVirt Jenkins CI Server

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: concurrency: Vdscli uses a new HTTP conn per req.
......................................................................

Patch Set 2: No score

Build Started http://jenkins.ovirt.org/job/vdsm_unit_tests_manual_gerrit/96/ (2/2)

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ifb08df759051bb340423958280e88df5792e9ed2
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Antoni Segura Puimedon 
Gerrit-Reviewer: Antoni Segura Puimedon 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Igor Lvovsky 
Gerrit-Reviewer: Livnat Peer 
Gerrit-Reviewer: Zhou Zheng Sheng 
Gerrit-Reviewer: oVirt Jenkins CI Server

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: concurrency: Vdscli uses a new HTTP conn per req.
......................................................................

Patch Set 2:

Build Successful

http://jenkins.ovirt.org/job/vdsm_pep8_gerrit/55/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_unit_tests_manual_gerrit/96/ : SUCCESS

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ifb08df759051bb340423958280e88df5792e9ed2
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Antoni Segura Puimedon 
Gerrit-Reviewer: Antoni Segura Puimedon 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Igor Lvovsky 
Gerrit-Reviewer: Livnat Peer 
Gerrit-Reviewer: Zhou Zheng Sheng 
Gerrit-Reviewer: oVirt Jenkins CI Server

[Dan Kenigsberg]

Dan Kenigsberg has posted comments on this change.

Change subject: concurrency: Vdscli uses a new HTTP conn per req.
......................................................................

Patch Set 2: Looks good to me, approved

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ifb08df759051bb340423958280e88df5792e9ed2
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Antoni Segura Puimedon 
Gerrit-Reviewer: Antoni Segura Puimedon 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Igor Lvovsky 
Gerrit-Reviewer: Livnat Peer 
Gerrit-Reviewer: Zhou Zheng Sheng 
Gerrit-Reviewer: oVirt Jenkins CI Server

[Dan Kenigsberg]

Dan Kenigsberg has posted comments on this change.

Change subject: concurrency: Vdscli uses a new HTTP conn per req.
......................................................................

Patch Set 2: Verified

I've seen no regressions on el6.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ifb08df759051bb340423958280e88df5792e9ed2
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Antoni Segura Puimedon 
Gerrit-Reviewer: Antoni Segura Puimedon 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Igor Lvovsky 
Gerrit-Reviewer: Livnat Peer 
Gerrit-Reviewer: Zhou Zheng Sheng 
Gerrit-Reviewer: oVirt Jenkins CI Server

[Dan Kenigsberg]

Dan Kenigsberg has submitted this change and it was merged.

Change subject: concurrency: Vdscli uses a new HTTP conn per req.
......................................................................

concurrency: Vdscli uses a new HTTP conn per req.

Python-2.7 changed xmlrpclib's implementation detail of spawining
a new http/https connection per each request to add support for
Keepalive. Unfortunately, this implementation detail was also what
made the xmlrpclib's Transport be thread-safe.

This change reverts the change in Transport, thus mantaining the
thread-safety of our python client code for unencrypted
communications.

Change-Id: Ifb08df759051bb340423958280e88df5792e9ed2
Signed-off-by: Antoni S. Puimedon 
---
M vdsm_cli/vdscli.py.in
1 file changed, 14 insertions(+), 1 deletion(-)

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

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

Gerrit-MessageType: merged
Gerrit-Change-Id: Ifb08df759051bb340423958280e88df5792e9ed2
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Antoni Segura Puimedon 
Gerrit-Reviewer: Antoni Segura Puimedon 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Igor Lvovsky 
Gerrit-Reviewer: Livnat Peer 
Gerrit-Reviewer: Zhou Zheng Sheng 
Gerrit-Reviewer: oVirt Jenkins CI Server