Libguestfs - Jun 2018 - Re: [PATCH] v2v: -o rhv-upload: Optimize http request sending

When sending request with small or no payload, it is simpler and
possibly more efficient to use the high level HTTPSConnection.request(),
instead of the lower level APIs.

The only reason to use the lower level APIs is to avoid copying the
payload, or on python 2, to use a bigger buffer size when streaming a
file-like object.
---
 v2v/rhv-upload-plugin.py | 35 ++++++++++++++++-------------------
 1 file changed, 16 insertions(+), 19 deletions(-)

diff --git a/v2v/rhv-upload-plugin.py b/v2v/rhv-upload-plugin.py
index ed99cc7a9..3fad865f6 100644
--- a/v2v/rhv-upload-plugin.py
+++ b/v2v/rhv-upload-plugin.py
@@ -263,12 +263,12 @@ def pread(h, count, offset):
     transfer = h['transfer']
     transfer_service = h['transfer_service']
 
-    http.putrequest("GET", h['path'])
+    headers = {"Range", "bytes=%d-%d" % (offset,
offset+count-1)}
     # Authorization is only needed for old imageio.
     if h['needs_auth']:
-        http.putheader("Authorization", transfer.signed_ticket)
-    http.putheader("Range", "bytes=%d-%d" % (offset,
offset+count-1))
-    http.endheaders()
+        headers["Authorization"] = transfer.signed_ticket
+
+    http.request("GET", h['path'], headers=headers)
 
     r = http.getresponse()
     # 206 = HTTP Partial Content.
@@ -319,11 +319,10 @@ def zero(h, count, offset, may_trim):
                       'size': count,
                       'flush': False}).encode()
 
-    http.putrequest("PATCH", h['path'])
-    http.putheader("Content-Type", "application/json")
-    http.putheader("Content-Length", len(buf))
-    http.endheaders()
-    http.send(buf)
+    headers = {"Content-Type": "application/json",
+               "Content-Length", str(len(buf))}
+
+    http.request("PATCH", h['path'], body=buf,
headers=headers)
 
     r = http.getresponse()
     if r.status != 200:
@@ -368,11 +367,10 @@ def trim(h, count, offset):
                       'size': count,
                       'flush': False}).encode()
 
-    http.putrequest("PATCH", h['path'])
-    http.putheader("Content-Type", "application/json")
-    http.putheader("Content-Length", len(buf))
-    http.endheaders()
-    http.send(buf)
+    headers = {"Content-Type": "application/json",
+               "Content-Length", str(len(buf))}
+
+    http.request("PATCH", h['path'], body=buf,
headers=headers)
 
     r = http.getresponse()
     if r.status != 200:
@@ -387,11 +385,10 @@ def flush(h):
     # Construct the JSON request for flushing.
     buf = json.dumps({'op': "flush"}).encode()
 
-    http.putrequest("PATCH", h['path'])
-    http.putheader("Content-Type", "application/json")
-    http.putheader("Content-Length", len(buf))
-    http.endheaders()
-    http.send(buf)
+    headers = {"Content-Type": "application/json",
+               "Content-Length", str(len(buf))}
+
+    http.request("PATCH", h['path'], body=buf,
headers=headers)
 
     r = http.getresponse()
     if r.status != 200:
-- 
2.17.1

On Thu, Jun 14, 2018 at 9:16 PM Nir Soffer <nirsof@gmail.com> wrote:
> When sending request with small or no payload, it is simpler and
> possibly more efficient to use the high level HTTPSConnection.request(),
> instead of the lower level APIs.
>
> The only reason to use the lower level APIs is to avoid copying the
> payload, or on python 2, to use a bigger buffer size when streaming a
> file-like object.
> ---
>  v2v/rhv-upload-plugin.py | 35 ++++++++++++++++-------------------
>  1 file changed, 16 insertions(+), 19 deletions(-)
>
> diff --git a/v2v/rhv-upload-plugin.py b/v2v/rhv-upload-plugin.py
> index ed99cc7a9..3fad865f6 100644
> --- a/v2v/rhv-upload-plugin.py
> +++ b/v2v/rhv-upload-plugin.py
> @@ -263,12 +263,12 @@ def pread(h, count, offset):
>      transfer = h['transfer']
>      transfer_service = h['transfer_service']
>
> -    http.putrequest("GET", h['path'])
> +    headers = {"Range", "bytes=%d-%d" % (offset,
offset+count-1)}
>      # Authorization is only needed for old imageio.
>      if h['needs_auth']:
> -        http.putheader("Authorization", transfer.signed_ticket)
> -    http.putheader("Range", "bytes=%d-%d" % (offset,
offset+count-1))
> -    http.endheaders()
> +        headers["Authorization"] = transfer.signed_ticket
> +
> +    http.request("GET", h['path'], headers=headers)
>
>      r = http.getresponse()
>      # 206 = HTTP Partial Content.
> @@ -319,11 +319,10 @@ def zero(h, count, offset, may_trim):
>                        'size': count,
>                        'flush': False}).encode()
>
> -    http.putrequest("PATCH", h['path'])
> -    http.putheader("Content-Type", "application/json")
> -    http.putheader("Content-Length", len(buf))
> -    http.endheaders()
> -    http.send(buf)
> +    headers = {"Content-Type": "application/json",
> +               "Content-Length", str(len(buf))}
> +
> +    http.request("PATCH", h['path'], body=buf,
headers=headers)
>
>      r = http.getresponse()
>      if r.status != 200:
> @@ -368,11 +367,10 @@ def trim(h, count, offset):
>                        'size': count,
>                        'flush': False}).encode()
>
> -    http.putrequest("PATCH", h['path'])
> -    http.putheader("Content-Type", "application/json")
> -    http.putheader("Content-Length", len(buf))
> -    http.endheaders()
> -    http.send(buf)
> +    headers = {"Content-Type": "application/json",
> +               "Content-Length", str(len(buf))}
> +
> +    http.request("PATCH", h['path'], body=buf,
headers=headers)
>
>      r = http.getresponse()
>      if r.status != 200:
> @@ -387,11 +385,10 @@ def flush(h):
>      # Construct the JSON request for flushing.
>      buf = json.dumps({'op': "flush"}).encode()
>
> -    http.putrequest("PATCH", h['path'])
> -    http.putheader("Content-Type", "application/json")
> -    http.putheader("Content-Length", len(buf))
> -    http.endheaders()
> -    http.send(buf)
> +    headers = {"Content-Type": "application/json",
> +               "Content-Length", str(len(buf))}
> +
> +    http.request("PATCH", h['path'], body=buf,
headers=headers)
>
>      r = http.getresponse()
>      if r.status != 200:
> --
> 2.17.1
>
WARNING: this is not tested, but I tested similar code here:
https://github.com/oVirt/ovirt-imageio/blob/master/examples/upload
(which need the same change).

Do we have an easy way to test the plugin without running the entire
virt-v2v pipeline?

Ideally, a way to run nbdkit with the plugin against an existing ovirt
system, and then use qemu-img to convert a file to the nbdkit nbd socket.

Nir

On Thu, Jun 14, 2018 at 09:16:01PM +0300, Nir Soffer
wrote:
> When sending request with small or no payload, it is simpler and
> possibly more efficient to use the high level HTTPSConnection.request(),
> instead of the lower level APIs.
> 
> The only reason to use the lower level APIs is to avoid copying the
> payload, or on python 2, to use a bigger buffer size when streaming a
> file-like object.
Change seems quite straightforward.  I'll have to test this out on my
own oVirt system first before I can properly verify it.

Thanks, Rich.
>  v2v/rhv-upload-plugin.py | 35 ++++++++++++++++-------------------
>  1 file changed, 16 insertions(+), 19 deletions(-)
> 
> diff --git a/v2v/rhv-upload-plugin.py b/v2v/rhv-upload-plugin.py
> index ed99cc7a9..3fad865f6 100644
> --- a/v2v/rhv-upload-plugin.py
> +++ b/v2v/rhv-upload-plugin.py
> @@ -263,12 +263,12 @@ def pread(h, count, offset):
>      transfer = h['transfer']
>      transfer_service = h['transfer_service']
>  
> -    http.putrequest("GET", h['path'])
> +    headers = {"Range", "bytes=%d-%d" % (offset,
offset+count-1)}
>      # Authorization is only needed for old imageio.
>      if h['needs_auth']:
> -        http.putheader("Authorization", transfer.signed_ticket)
> -    http.putheader("Range", "bytes=%d-%d" % (offset,
offset+count-1))
> -    http.endheaders()
> +        headers["Authorization"] = transfer.signed_ticket
> +
> +    http.request("GET", h['path'], headers=headers)
>  
>      r = http.getresponse()
>      # 206 = HTTP Partial Content.
> @@ -319,11 +319,10 @@ def zero(h, count, offset, may_trim):
>                        'size': count,
>                        'flush': False}).encode()
>  
> -    http.putrequest("PATCH", h['path'])
> -    http.putheader("Content-Type", "application/json")
> -    http.putheader("Content-Length", len(buf))
> -    http.endheaders()
> -    http.send(buf)
> +    headers = {"Content-Type": "application/json",
> +               "Content-Length", str(len(buf))}
> +
> +    http.request("PATCH", h['path'], body=buf,
headers=headers)
>  
>      r = http.getresponse()
>      if r.status != 200:
> @@ -368,11 +367,10 @@ def trim(h, count, offset):
>                        'size': count,
>                        'flush': False}).encode()
>  
> -    http.putrequest("PATCH", h['path'])
> -    http.putheader("Content-Type", "application/json")
> -    http.putheader("Content-Length", len(buf))
> -    http.endheaders()
> -    http.send(buf)
> +    headers = {"Content-Type": "application/json",
> +               "Content-Length", str(len(buf))}
> +
> +    http.request("PATCH", h['path'], body=buf,
headers=headers)
>  
>      r = http.getresponse()
>      if r.status != 200:
> @@ -387,11 +385,10 @@ def flush(h):
>      # Construct the JSON request for flushing.
>      buf = json.dumps({'op': "flush"}).encode()
>  
> -    http.putrequest("PATCH", h['path'])
> -    http.putheader("Content-Type", "application/json")
> -    http.putheader("Content-Length", len(buf))
> -    http.endheaders()
> -    http.send(buf)
> +    headers = {"Content-Type": "application/json",
> +               "Content-Length", str(len(buf))}
> +
> +    http.request("PATCH", h['path'], body=buf,
headers=headers)
>  
>      r = http.getresponse()
>      if r.status != 200:
> -- 
> 2.17.1
-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-top is 'top' for virtual machines.  Tiny program with many
powerful monitoring features, net stats, disk stats, logging, etc.
http://people.redhat.com/~rjones/virt-top

On Thu, Jun 14, 2018 at 10:19 PM Richard W.M. Jones <rjones@redhat.com>
wrote:
> On Thu, Jun 14, 2018 at 09:16:01PM +0300, Nir Soffer wrote:
> > When sending request with small or no payload, it is simpler and
> > possibly more efficient to use the high level
HTTPSConnection.request(),
> > instead of the lower level APIs.
> >
> > The only reason to use the lower level APIs is to avoid copying the
> > payload, or on python 2, to use a bigger buffer size when streaming a
> > file-like object.
>
> Change seems quite straightforward.  I'll have to test this out on my
> own oVirt system first before I can properly verify it.
>
I tested the same change with imageio example upload script, and
it *doubled* the throughput.

See https://gerrit.ovirt.org/#/c/92275/

Thanks, Rich.
>
> >  v2v/rhv-upload-plugin.py | 35 ++++++++++++++++-------------------
> >  1 file changed, 16 insertions(+), 19 deletions(-)
> >
> > diff --git a/v2v/rhv-upload-plugin.py b/v2v/rhv-upload-plugin.py
> > index ed99cc7a9..3fad865f6 100644
> > --- a/v2v/rhv-upload-plugin.py
> > +++ b/v2v/rhv-upload-plugin.py
> > @@ -263,12 +263,12 @@ def pread(h, count, offset):
> >      transfer = h['transfer']
> >      transfer_service = h['transfer_service']
> >
> > -    http.putrequest("GET", h['path'])
> > +    headers = {"Range", "bytes=%d-%d" % (offset,
offset+count-1)}
> >      # Authorization is only needed for old imageio.
> >      if h['needs_auth']:
> > -        http.putheader("Authorization",
transfer.signed_ticket)
> > -    http.putheader("Range", "bytes=%d-%d" %
(offset, offset+count-1))
> > -    http.endheaders()
> > +        headers["Authorization"] = transfer.signed_ticket
> > +
> > +    http.request("GET", h['path'], headers=headers)
> >
> >      r = http.getresponse()
> >      # 206 = HTTP Partial Content.
> > @@ -319,11 +319,10 @@ def zero(h, count, offset, may_trim):
> >                        'size': count,
> >                        'flush': False}).encode()
> >
> > -    http.putrequest("PATCH", h['path'])
> > -    http.putheader("Content-Type",
"application/json")
> > -    http.putheader("Content-Length", len(buf))
> > -    http.endheaders()
> > -    http.send(buf)
> > +    headers = {"Content-Type":
"application/json",
> > +               "Content-Length", str(len(buf))}
> > +
> > +    http.request("PATCH", h['path'], body=buf,
headers=headers)
> >
> >      r = http.getresponse()
> >      if r.status != 200:
> > @@ -368,11 +367,10 @@ def trim(h, count, offset):
> >                        'size': count,
> >                        'flush': False}).encode()
> >
> > -    http.putrequest("PATCH", h['path'])
> > -    http.putheader("Content-Type",
"application/json")
> > -    http.putheader("Content-Length", len(buf))
> > -    http.endheaders()
> > -    http.send(buf)
> > +    headers = {"Content-Type":
"application/json",
> > +               "Content-Length", str(len(buf))}
> > +
> > +    http.request("PATCH", h['path'], body=buf,
headers=headers)
> >
> >      r = http.getresponse()
> >      if r.status != 200:
> > @@ -387,11 +385,10 @@ def flush(h):
> >      # Construct the JSON request for flushing.
> >      buf = json.dumps({'op': "flush"}).encode()
> >
> > -    http.putrequest("PATCH", h['path'])
> > -    http.putheader("Content-Type",
"application/json")
> > -    http.putheader("Content-Length", len(buf))
> > -    http.endheaders()
> > -    http.send(buf)
> > +    headers = {"Content-Type":
"application/json",
> > +               "Content-Length", str(len(buf))}
> > +
> > +    http.request("PATCH", h['path'], body=buf,
headers=headers)
> >
> >      r = http.getresponse()
> >      if r.status != 200:
> > --
> > 2.17.1
>
> --
> Richard Jones, Virtualization Group, Red Hat
> http://people.redhat.com/~rjones
> Read my programming and virtualization blog: http://rwmj.wordpress.com
> virt-top is 'top' for virtual machines.  Tiny program with many
> powerful monitoring features, net stats, disk stats, logging, etc.
> http://people.redhat.com/~rjones/virt-top
>

On Thu, 14 Jun 2018 21:16:01 +0300
Nir Soffer <nirsof@gmail.com> wrote:
> When sending request with small or no payload, it is simpler and
> possibly more efficient to use the high level HTTPSConnection.request(),
> instead of the lower level APIs.
> 
> The only reason to use the lower level APIs is to avoid copying the
> payload, or on python 2, to use a bigger buffer size when streaming a
> file-like object.
> ---
>  v2v/rhv-upload-plugin.py | 35 ++++++++++++++++-------------------
>  1 file changed, 16 insertions(+), 19 deletions(-)
FWIW LGTM

On Thu, Jun 14, 2018 at 09:24:48PM +0300, Nir Soffer
wrote:
> On Thu, Jun 14, 2018 at 9:16 PM Nir Soffer <nirsof@gmail.com> wrote:
> > +    headers = {"Content-Type":
"application/json",
> > +               "Content-Length", str(len(buf))}
There were a few Python syntax errors such as this one.  They
can be found by running:

  make -C v2v check TESTS=test-v2v-python-syntax.sh

However I have now fixed them.
> Do we have an easy way to test the plugin without running the entire
> virt-v2v pipeline?
Not really.  In fact we don't have any unit tests for -o rhv-upload
functionality because there's no way to simulate the imageio server.

However you can run virt-v2v locally against an oVirt instance without
needing VMware.  The command is rather lengthy, but here it is:

$ virt-builder fedora-27
$ ./run virt-v2v -i disk /var/tmp/fedora-27.img \
    -o rhv-upload \
    -oc https://ovirt-engine.example.com/ovirt-engine/api \
    -os ovirt-data \
    -op /tmp/password \
    -of raw \
    -oo rhv-cafile=/tmp/ca.pem \
    -oo rhv-direct

/tmp/password should contain the oVirt admin password.
/tmp/ca.pem should contain the oVirt CA cert.

This will create a guest called ‘fedora-27’ which you'll need to
delete (on oVirt) afterwards.  You can add ‘-on name’ to name it
something else.

In any case I have fixed and verified this patch and will push it
soon, thanks.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-builder quickly builds VMs from scratch
http://libguestfs.org/virt-builder.1.html