Richard W.M. Jones
2018-Nov-20 17:04 UTC
Re: [Libguestfs] [PATCH v2 3/3] v2v: -o rhv-upload: Add a test.
On Mon, Oct 15, 2018 at 05:13:31PM +0100, Richard W.M. Jones wrote:> On Mon, Oct 15, 2018 at 06:43:10PM +0300, Nir Soffer wrote: > > On Mon, Oct 15, 2018 at 6:21 PM Richard W.M. Jones <rjones@redhat.com> > > wrote: > > > > > On Tue, Oct 09, 2018 at 02:28:10PM +0300, Nir Soffer wrote: > > > > > +# Create a background thread running a web server which is > > > > > +# simulating the imageio server. > > > > > > > > > > > > > This functionality should be separated from the fake SDK module, since > > > it is > > > > not part of the SDK, and may be replaced by real imageio server later. > > > > > > Well possibly, but it was very convenient to put it in the class here, > > > and this test is meant for running completely standalone without any > > > other service available. > > > > > > > > +class RequestHandler(BaseHTTPRequestHandler): > > > > > > > > > > > > > This request handler is using HTTP/1.0, and will close the connection > > > after > > > > every request. This is not a good implementation of the imageio server, > > > and > > > > also > > > > hides bugs in this code. > > > > > > > > Should be fixed by adding: > > > > > > > > protocol_version = "HTTP/1.1" > > > > > > I tried the attached patch, but for some reason it just hangs. > > > > > > > The issue is probably missing "content-length: 0" header in the > > response. If we don't close the connection and don't send > > content-length the client cannot do much but wait :-) > > Ah yes, it was missing from the OPTIONS method. > The attached patch does work. What do you think? > > 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> >From 6eb473a7e67de82bf08b97f2610ee3364071c741 Mon Sep 17 00:00:00 2001 > From: "Richard W.M. Jones" <rjones@redhat.com> > Date: Mon, 15 Oct 2018 16:07:52 +0100 > Subject: [PATCH] v2v: -o rhv-upload: Test using HTTP/1.1 protocol. > > Fixes commit b54b58c44e3f1f54a05117c758eaa21d9f1085f0. > > Thanks: Nir Soffer. > --- > .../ovirtsdk4/__init__.py | 28 ++++++++++++++++--- > 1 file changed, 24 insertions(+), 4 deletions(-) > > diff --git a/v2v/test-v2v-o-rhv-upload-module/ovirtsdk4/__init__.py b/v2v/test-v2v-o-rhv-upload-module/ovirtsdk4/__init__.py > index b5f739471..84b9d56aa 100644 > --- a/v2v/test-v2v-o-rhv-upload-module/ovirtsdk4/__init__.py > +++ b/v2v/test-v2v-o-rhv-upload-module/ovirtsdk4/__init__.py > @@ -116,24 +116,44 @@ from http.server import HTTPServer, BaseHTTPRequestHandler > import threading > > class RequestHandler(BaseHTTPRequestHandler): > + protocol_version = 'HTTP/1.1' > + > def do_OPTIONS(self): > - self.send_response(200) > - self.send_header("Content-type", "application/json; charset=UTF-8") > - self.end_headers() > + self.discard_request() > + > # Advertize only zero support. > - self.wfile.write(b'''{ "features": [ "zero" ] }''') > + content = b'''{ "features": [ "zero" ] }''' > + length = len(content) > + > + self.send_response(200) > + self.send_header("Content-type", "application/json; charset=UTF-8") > + self.send_header("Content-Length", length) > + self.end_headers() > + self.wfile.write(content) > > # eg. zero request. Just ignore it. > def do_PATCH(self): > + self.discard_request() > self.send_response(200) > + self.send_header("Content-Length", "0") > self.end_headers() > > # Flush request. Ignore it. > def do_PUT(self): > + self.discard_request() > self.send_response(200) > + self.send_header("Content-Length", "0") > self.end_headers() > > + def discard_request(self): > + length = self.headers['Content-Length'] > + if length: > + length = int(length) > + content = self.rfile.read(length)This commit went upstream: https://github.com/libguestfs/libguestfs/commit/bfd9bee9fc7a91fdca0ca051deca062ff29d1f01 However it fails on RHEL 7.6 with: Traceback (most recent call last): File "/usr/lib64/python2.7/SocketServer.py", line 295, in _handle_request_nobl ock self.process_request(request, client_address) File "/usr/lib64/python2.7/SocketServer.py", line 321, in process_request self.finish_request(request, client_address) File "/usr/lib64/python2.7/SocketServer.py", line 334, in finish_request self.RequestHandlerClass(request, client_address, self) File "/usr/lib64/python2.7/SocketServer.py", line 649, in __init__ self.handle() File "/usr/lib64/python2.7/BaseHTTPServer.py", line 340, in handle self.handle_one_request() File "/usr/lib64/python2.7/BaseHTTPServer.py", line 328, in handle_one_request method() File "/home/rjones/d/libguestfs-rhel-7.6-lp/v2v/test-v2v-o-rhv-upload-module/o virtsdk4/__init__.py", line 123, in do_OPTIONS self.discard_request() File "/home/rjones/d/libguestfs-rhel-7.6-lp/v2v/test-v2v-o-rhv-upload-module/ovirtsdk4/__init__.py", line 150, in discard_request length = self.headers['Content-Length'] File "/usr/lib64/python2.7/rfc822.py", line 388, in __getitem__ return self.dict[name.lower()] KeyError: 'content-length' I guess that Python won't let us do: length = self.headers['Content-Length'] if length: How about the attached fix? Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com virt-df lists disk usage of guests without needing to install any software inside the virtual machine. Supports Linux and Windows. http://people.redhat.com/~rjones/virt-df/
Richard W.M. Jones
2018-Nov-20 17:49 UTC
Re: [Libguestfs] [PATCH v2 3/3] v2v: -o rhv-upload: Add a test.
On Tue, Nov 20, 2018 at 05:04:15PM +0000, Richard W.M. Jones wrote:> >From 9aca67b9875ab31bea7f3aeec411dad3ed990f17 Mon Sep 17 00:00:00 2001 > From: "Richard W.M. Jones" <rjones@redhat.com> > Date: Tue, 20 Nov 2018 17:00:33 +0000 > Subject: [PATCH] v2v: -o rhv-upload: Fix test so it doesn't fail if > Content-Length header missing. > > If the Content-Length header was missing from the headers returned by > the server then the test would fail with: > > KeyError: 'content-length' > > This happens on RHEL 7.6 in particular. We can fix this by using an > alternate method to fetch the header, which will return None instead > of throwing an exception if the header doesn't exist. See: > https://stackoverflow.com/a/19255675 > --- > v2v/test-v2v-o-rhv-upload-module/ovirtsdk4/__init__.py | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/v2v/test-v2v-o-rhv-upload-module/ovirtsdk4/__init__.py b/v2v/test-v2v-o-rhv-upload-module/ovirtsdk4/__init__.py > index 84b9d56aa..8d1058d67 100644 > --- a/v2v/test-v2v-o-rhv-upload-module/ovirtsdk4/__init__.py > +++ b/v2v/test-v2v-o-rhv-upload-module/ovirtsdk4/__init__.py > @@ -146,7 +146,7 @@ class RequestHandler(BaseHTTPRequestHandler): > self.end_headers() > > def discard_request(self): > - length = self.headers['Content-Length'] > + length = self.headers.get('Content-Length') > if length: > length = int(length) > content = self.rfile.read(length) > -- > 2.19.0.rc0FYI I have tested this patch on Fedora 29 & RHEL 7 and the test now succeeds on both. Previously the test failed on RHEL 7. Rich. -- 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
Nir Soffer
2018-Nov-21 15:47 UTC
Re: [Libguestfs] [PATCH v2 3/3] v2v: -o rhv-upload: Add a test.
On Tue, Nov 20, 2018 at 7:49 PM Richard W.M. Jones <rjones@redhat.com> wrote:> On Tue, Nov 20, 2018 at 05:04:15PM +0000, Richard W.M. Jones wrote: > > >From 9aca67b9875ab31bea7f3aeec411dad3ed990f17 Mon Sep 17 00:00:00 2001 > > From: "Richard W.M. Jones" <rjones@redhat.com> > > Date: Tue, 20 Nov 2018 17:00:33 +0000 > > Subject: [PATCH] v2v: -o rhv-upload: Fix test so it doesn't fail if > > Content-Length header missing. > > > > If the Content-Length header was missing from the headers returned by > > the server then the test would fail with: > > > > KeyError: 'content-length' > > > > This happens on RHEL 7.6 in particular. We can fix this by using an > > alternate method to fetch the header, which will return None instead > > of throwing an exception if the header doesn't exist. See: > > https://stackoverflow.com/a/19255675 > > --- > > v2v/test-v2v-o-rhv-upload-module/ovirtsdk4/__init__.py | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/v2v/test-v2v-o-rhv-upload-module/ovirtsdk4/__init__.py > b/v2v/test-v2v-o-rhv-upload-module/ovirtsdk4/__init__.py > > index 84b9d56aa..8d1058d67 100644 > > --- a/v2v/test-v2v-o-rhv-upload-module/ovirtsdk4/__init__.py > > +++ b/v2v/test-v2v-o-rhv-upload-module/ovirtsdk4/__init__.py > > @@ -146,7 +146,7 @@ class RequestHandler(BaseHTTPRequestHandler): > > self.end_headers() > > > > def discard_request(self): > > - length = self.headers['Content-Length'] > > + length = self.headers.get('Content-Length') > > if length: > > length = int(length) > > content = self.rfile.read(length) > > -- > > 2.19.0.rc0 >Looks fine. Nir> > FYI I have tested this patch on Fedora 29 & RHEL 7 and the test now > succeeds on both. Previously the test failed on RHEL 7. > > Rich. > > -- > 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 >