On Tue, Jun 26, 2018 at 9:38 PM Nir Soffer <[email protected]> wrote:
> On Tue, Jun 26, 2018 at 4:25 PM Richard W.M. Jones <[email protected]> > wrote: > >> In the case where virt-v2v runs on the same server as the imageio >> daemon that we are talking to, it may be possible to optimize access >> using a Unix domain socket. >> >> This is only an optimization. If it fails or if we're not running on >> the same server it will fall back to the usual HTTPS over TCP >> connection. >> >> Thanks: Nir Soffer, Daniel Erez. >> --- >> v2v/rhv-upload-plugin.py | 61 >> +++++++++++++++++++++++++++++++++++++++++++++--- >> 1 file changed, 58 insertions(+), 3 deletions(-) >> >> diff --git a/v2v/rhv-upload-plugin.py b/v2v/rhv-upload-plugin.py >> index f215eaecf..354cc524c 100644 >> --- a/v2v/rhv-upload-plugin.py >> +++ b/v2v/rhv-upload-plugin.py >> @@ -19,11 +19,12 @@ >> import builtins >> import json >> import logging >> +import socket >> import ssl >> import sys >> import time >> >> -from http.client import HTTPSConnection >> +from http.client import HTTPSConnection, HTTPConnection >> from urllib.parse import urlparse >> >> import ovirtsdk4 as sdk >> @@ -56,6 +57,28 @@ def debug(s): >> print(s, file=sys.stderr) >> sys.stderr.flush() >> >> +def find_host(connection): >> + """Return the current host object or None.""" >> > > I think we need a comment for this, something like: > > # If we are running on a oVirt host, it must have a hardware id > # file. > > >> + try: >> + with builtin_open("/etc/vdsm/vdsm.id") as f: >> + vdsm_id = f.readline().strip() >> + except Exception as e: >> + return None > > > > > A debug message about no hardware id file would be nice to the person > looking at the logs later. Without this you will have to detect this case > by not seeing the next debug line. Possible but require more effort. > I tested the patch yesterday, and unix socket did not work - silently. After changing the code to: try: with builtin_open("/etc/vdsm/vdsm.id") as f: vdsm_id = f.readline().strip() except Exception as e: debug("cannot read host hardware id: %s" % e) return None Then I got this message: cannot read host hardware id: name 'builtin_open' is not defined We are paying the price of python :-) > > >> + debug("hw_id = %r" % vdsm_id) >> + >> > > A comment abut looking up the host id would be nice. The code is not > explaining it self very well. > > >> + hosts_service = connection.system_service().hosts_service() >> + hosts = hosts_service.list( >> + search="hw_id=%s" % vdsm_id, >> + case_sensitive=False, >> + ) >> + if len(hosts) == 0: >> + return None >> > > Debug message about unknown host would be nice. > > Did you try to feed non existing hardware id? do we actually get empty > list? > > >> + >> + host = hosts[0] >> + debug("host.id = %r" % host.id) >> + >> + return types.Host(id = host.id) >> > > I like this, simple and easy to follow. > > >> + >> def open(readonly): >> # Parse out the username from the output_conn URL. >> parsed = urlparse(params['output_conn']) >> @@ -121,9 +144,11 @@ def open(readonly): >> transfers_service = system_service.image_transfers_service() >> >> # Create a new image transfer. >> + host = find_host(connection) >> transfer = transfers_service.add( >> types.ImageTransfer( >> disk = types.Disk(id = disk.id), >> + host = host, >> inactivity_timeout = 3600, >> ) >> > > Nice! > > >> ) >> @@ -170,6 +195,7 @@ def open(readonly): >> can_flush = False >> can_trim = False >> can_zero = False >> + unix_socket = None >> >> http.putrequest("OPTIONS", destination_url.path) >> http.putheader("Authorization", transfer.signed_ticket) >> @@ -186,6 +212,7 @@ def open(readonly): >> can_flush = "flush" in j['features'] >> can_trim = "trim" in j['features'] >> can_zero = "zero" in j['features'] >> + unix_socket = j.get('unix_socket') >> >> # Old imageio servers returned either 405 Method Not Allowed or >> # 204 No Content (with an empty body). If we see that we leave >> @@ -197,8 +224,17 @@ def open(readonly): >> raise RuntimeError("could not use OPTIONS request: %d: %s" % >> (r.status, r.reason)) >> >> - debug("imageio features: flush=%r trim=%r zero=%r" % >> - (can_flush, can_trim, can_zero)) >> + debug("imageio features: flush=%r trim=%r zero=%r unix_socket=%r" % >> + (can_flush, can_trim, can_zero, unix_socket)) >> + >> + # If we are connected to imageio on the local host and the >> + # transfer features a unix_socket then we can reconnect to that. >> + if host is not None and unix_socket is not None: >> + try: >> + http = UnixHTTPConnection(unix_socket) >> + debug("optimizing connection using unix socket %r" % >> unix_socket) >> + except: >> > > I think this should be: > > except Exception as e: > debug("error using unix socket connection, using https connection: %s" > % e) > > >> + pass >> >> # Save everything we need to make requests in the handle. >> return { >> @@ -463,3 +499,22 @@ def close(h): >> raise >> >> connection.close() >> + >> +# Modify http.client.HTTPConnection to work over a Unix domain socket. >> +# Derived from uhttplib written by Erik van Zijst under an MIT license. >> +# (https://pypi.org/project/uhttplib/) >> +# Ported to Python 3 by Irit Goihman. >> + >> +class UnsupportedError(Exception): >> + pass >> > > Yes this is unneeded now. > > >> + >> +class UnixHTTPConnection(HTTPConnection): >> + def __init__(self, path, timeout=socket._GLOBAL_DEFAULT_TIMEOUT): >> + self.path = path >> + HTTPConnection.__init__(self, "localhost", timeout=timeout) >> + >> + def connect(self): >> + self.sock = socket.socket(socket.AF_UNIX, socket.SOCK_STREAM) >> + if self.timeout is not socket._GLOBAL_DEFAULT_TIMEOUT: >> + self.sock.settimeout(timeout) >> + self.sock.connect(self.path) >> -- >> 2.16.2 >> > >
_______________________________________________ Libguestfs mailing list [email protected] https://www.redhat.com/mailman/listinfo/libguestfs
