[PATCH v3] v2v: -o rhv-upload: Use Unix domain socket to access imageio (RHBZ#1588088).
v2 was here: https://www.redhat.com/archives/libguestfs/2018-June/msg00109.html v3: - Added/fixed all suggestions from Nir in previous review. Q: I wasn't sure if we still need the "UnsupportedError" class so I left it in. Q: Does the Unix socket always have the same name? What happens if there's more than one transfer happening? I tested this both ways, and it worked both ways. The output when the optimization is used looks like this: disk.id = '45d1a30c-82c0-4b00-8bfb-c4d628dbf72d' hw_id = '81F716C4-4D19-CB35-82F7-94C691189FEB' host.id = '87daa64d-b27e-4336-a156-973544be708b' transfer.id = '5ffea7fa-966c-42a4-a77a-3efd6dc45c34' imageio features: flush=True trim=False zero=True unix_socket='\x00/org/ovirt/imageio' optimizing connection using unix socket '\x00/org/ovirt/imageio' Rich.
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.""" + try: + with builtin_open("/etc/vdsm/vdsm.id") as f: + vdsm_id = f.readline().strip() + except Exception as e: + return None + debug("hw_id = %r" % vdsm_id) + + 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 + + host = hosts[0] + debug("host.id = %r" % host.id) + + return types.Host(id = host.id) + 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, ) ) @@ -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: + 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 + +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
On Tue, Jun 26, 2018 at 4:25 PM Richard W.M. Jones <rjones@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.
+ 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
On Tue, Jun 26, 2018 at 9:38 PM Nir Soffer <nsoffer@redhat.com> wrote:
On Tue, Jun 26, 2018 at 4:25 PM Richard W.M. Jones <rjones@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
On Wed, Jun 27, 2018 at 12:37:20PM +0300, Nir Soffer wrote:
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 :-)
Ah, that's a typo. The Python 3 version is meant to use builtins.open. However confusingly I was testing the Python 2 version which has a RHEL-only patch that does: -import builtins +from __builtin__ import open as builtin_open (https://github.com/libguestfs/libguestfs/commit/e5826e63b53a0aadc8a8d5261ebf...) Because I wasn't expecting the Python 3 version to work, I didn't notice this error. And yes you're right, Strong Typing FTW. It's why we use OCaml where possible. I will fix this. Were there any other problems? Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com virt-p2v converts physical machines to virtual machines. Boot with a live CD or over the network (PXE) and turn machines into KVM guests. http://libguestfs.org/virt-v2v
On Wed, Jun 27, 2018 at 2:10 PM Richard W.M. Jones <rjones@redhat.com> wrote:
On Wed, Jun 27, 2018 at 12:37:20PM +0300, Nir Soffer wrote:
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 :-)
Ah, that's a typo.
The Python 3 version is meant to use builtins.open.
However confusingly I was testing the Python 2 version which has a RHEL-only patch that does:
-import builtins +from __builtin__ import open as builtin_open
( https://github.com/libguestfs/libguestfs/commit/e5826e63b53a0aadc8a8d5261ebf... )
Because I wasn't expecting the Python 3 version to work, I didn't notice this error.
And yes you're right, Strong Typing FTW. It's why we use OCaml where possible.
I will fix this. Were there any other problems?
It worked fine.
Rich.
-- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com virt-p2v converts physical machines to virtual machines. Boot with a live CD or over the network (PXE) and turn machines into KVM guests. http://libguestfs.org/virt-v2v
participants (2)
-
Nir Soffer -
Richard W.M. Jones