[Libguestfs] [PATCH v3] v2v: -o rhv-upload: Use Unix domain socket to access imageio (RHBZ#1588088).
Nir Soffer
nsoffer at redhat.com
Wed Jun 27 09:37:20 UTC 2018
On Tue, Jun 26, 2018 at 9:38 PM Nir Soffer <nsoffer at redhat.com> wrote:
> On Tue, Jun 26, 2018 at 4:25 PM Richard W.M. Jones <rjones at redhat.com>
> 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
>>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://listman.redhat.com/archives/libguestfs/attachments/20180627/fe9e83e3/attachment.htm>
More information about the Libguestfs
mailing list