Libguestfs - Jul 2020 - Re: [PATCH] RFC: rhv-upload-plugin: Use imageio client

On Thu, Jul 9, 2020 at 10:53 AM Richard W.M. Jones <rjones@redhat.com>
wrote:
>
> On Thu, Jul 09, 2020 at 01:51:44AM +0300, Nir Soffer wrote:
> > We can use now ImageioClient to communicate with ovirt-imageio server
> > on oVirt host.
> >
> > Using the client greatly simplifies the plugin, and enables new
features
> > like transparent proxy support. The client will use transfer_url if
> > possible, or fall back to proxy_url.
> >
> > Since the client implements the buffer protocol, move to version 2 of
> > the API for more efficient pread().
>
> This will require nbdkit >= 1.17.3 which implies RHEL AV 8.3.0, but
> that's fine since we only ship the new virt-v2v (>= 1.42) on AV
8.3.0.
I'll update the spec if it is in the source then.
> > Another advantage the client is maintained by oVirt, and fixes are
> > delivered quickly in oVirt, without depending on RHEL release
schedule.
> >
> > Not ready yet, we have several issues:
> >
> > - The client does not support "http", so the tests will fail
now. This
> >   is good since we should test with real imageio server. I will work
on
> >   better tests later.
>
> I think having standalone tests is still worthwhile as it's the only
> way that the plugin gets tested on a regular basis.  IIRC at the
> moment we only test against a faked ovirt SDK.  I guess this would be
> easy to adjust?
Testing with a real server is easy. I have incomplete patch using
imageio server with some manual setup.

This is the setup needed:

$ git clone https://github.com/oVirt/ovirt-imageio.git

In another shell, start the server:

$ cd ovirt-imageio/daemon

$ ./ovirt-imageio -c test
2020-07-09 02:55:57,839 INFO    (MainThread) [server] Starting
(pid=200640, version=2.0.10)
2020-07-09 02:55:57,842 INFO    (MainThread) [services] remote.service
listening on ('::', 54322)
2020-07-09 02:55:57,842 INFO    (MainThread) [services] local.service
listening on '\x00/org/ovirt/imageio'
2020-07-09 02:55:57,843 INFO    (MainThread) [services]
control.service listening on 'test/daemon.sock'
2020-07-09 02:55:57,844 INFO    (MainThread) [server] Ready for requests

To make uploads possible, we need to instal a ticket matching the
transfer_url:

    $ curl --unix-socket ovirt-imageio/daemon/test/daemon.sock \
        -X PUT \
        --upload-file tests/test-v2v-o-rhv-upload-module/file.json \
        http://localhost/tickets/test

Finally we need to create the target image:

    qemu-img create -f raw /var/tmp/test.raw 512m

With this we can run virt-v2v, and since we use a real server, we can
check that the image was imported correctly:

    qemu-img compare overlay.qcow2 /var/tmp/test.raw

To test imageio ndb support, need to import to qcow2 disks, we need to
install the nbd ticket:

    $ curl --unix-socket ovirt-imageio/daemon/test/daemon.sock \
        -X PUT \
        --upload-file tests/test-v2v-o-rhv-upload-module/nbd.json \
        http://localhost/tickets/test

Now we can create qcow2 target image:

    qemu-img create -f qcow2 /var/tmp/test.qcow2 512m

And run also qemu-nbd:

    $ qemu-nbd --socket=/tmp/nbd.sock \
        --persistent \
        --shared=8 \
        --format=qcow2 \
        --aio=native \
        --cache=none \
        --discard=unmap \
        /var/tmp/test.qcow2

With this manual setup, the rhv-upload test works for both raw and qcow2
target disks.

$ cat tests/test-v2v-o-rhv-upload-module/file.json
{
    "uuid": "test",
    "size": 536870912,
    "url": "file:///var/tmp/test.raw",
    "timeout": 3000,
    "sparse": true,
    "ops": ["write"]
}

$ cat tests/test-v2v-o-rhv-upload-module/nbd.json
{
    "uuid": "test",
    "size": 536870912,
    "url": "nbd:unix:/tmp/nbd.sock",
    "timeout": 3000,
    "sparse": true,
    "ops": ["write"]
}

If we want to test with a fake, we can create fake client - no need for runing
http sever:

class FakeImageioClient:

    def write(self, offset, buf):
        pass

    ...

This is easy, but we don't test much.

I can start with replacing the fake server with fake client, and document
somewhere how to test using a real server.
> To unit test against a real imageio server is difficult I think: These
> tests would need to run in Fedora with minimal large dependencies.  We
> might create a fake web server like the one we use in nbdkit:
>
> https://github.com/libguestfs/nbdkit/blob/master/tests/web-server.h
> https://github.com/libguestfs/nbdkit/blob/master/tests/web-server.c
>
> This only supports http but we could put stunnel in front to provide
> https.
I don't think this is the right approach. We already failed last time when
we added the fake http server  the tests passed but the change to support
them broke the real code.

Wrong tests are the worst.
> > - Need to require ovirt-imageio-client, providing the client library.
>
> That's a simple change in virt-v2v packaging.  I don't see this
> package in Fedora Koji.
Is this an issue? oVirt is not really supported on Fedora these days.

I can try to add imageio to Fedora, this is a good idea anyway. We don't
have any dependencies on other oVirt components now.
> In RHEL I can see the package and the
> dependencies look quite light, basically just Python and python3-six.
> Why is it only available for x86_64 and ppc64le?
Probably because RHV is not built for other arches.

Is this an issue? e.g. do we support importing vms in virt-v2v runing on
arm, importing to oVirt/RHV running on x86/ppc?
> > - params['rhv_direct'] is ignored, we always try direct
transfer now.
>
> We should drop it from the OCaml code?
And break users that used this flag?
> [...]
>
> > -# 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 UnixHTTPConnection(HTTPConnection):
>
> Why drop this part?
Not used now, imageio client includes this class:
https://github.com/oVirt/ovirt-imageio/blob/24c59f2e0ace784d9c993f6044475bb370058e70/daemon/ovirt_imageio/_internal/backends/http.py#L610
>
> Rest of the patch looks good and as you say above both simplifies and
> improves performance.
The performance part was not tested yet. I will results in the next version.

Nir

On Thu, Jul 09, 2020 at 03:30:22PM +0300, Nir Soffer
wrote:
> Testing with a real server is easy. I have incomplete patch using
> imageio server with some manual setup.
[...]

For a certain definition of "easy" :-)

Our QE team tests -o rhv-upload from time to time, and will do an
end-to-end test of the system when RHV 4.4 arrives with the new
feature.

For “make check” we need something that will run in a few seconds and
doesn't have any major dependencies.
> If we want to test with a fake, we can create fake client - no need for
runing
> http sever:
> 
> class FakeImageioClient:
> 
>     def write(self, offset, buf):
>         pass
> 
>     ...
> 
> This is easy, but we don't test much.
Right, this is what I was thinking of for “make check”.  While it's
not a comprehensive test, it is still testing something valuable:
whether the Python code can compile and run from beginning to end
without runtime errors.
> I don't think this is the right approach. We already failed last time
when
> we added the fake http server  the tests passed but the change to support
> them broke the real code.
>
> Wrong tests are the worst.
Agreed.
> > > - Need to require ovirt-imageio-client, providing the client
library.
> >
> > That's a simple change in virt-v2v packaging.  I don't see
this
> > package in Fedora Koji.
> 
> Is this an issue? oVirt is not really supported on Fedora these days.
No it's not an issue, but the feature will not work on Fedora.  A bit
surprised though - wouldn't you want people to be able to upload disk
images to oVirt from their Fedora clients?

...
> > In RHEL I can see the package and the
> > dependencies look quite light, basically just Python and python3-six.
> > Why is it only available for x86_64 and ppc64le?
> 
> Probably because RHV is not built for other arches.
> 
> Is this an issue? e.g. do we support importing vms in virt-v2v runing on
> arm, importing to oVirt/RHV running on x86/ppc?
Virt-v2v is only built for x86-64 in RHEL 8, so it's not really an issue.

I was more wondering if there was some problem that prevented it from
working on the other architectures thinking about the Fedora case, but
if we're not going to package this up in Fedora it doesn't matter.
> > > - params['rhv_direct'] is ignored, we always try direct
transfer now.
> >
> > We should drop it from the OCaml code?
> 
> And break users that used this flag?
Actually I meant just drop the params['rhv_direct'] field, ie:
https://github.com/libguestfs/virt-v2v/blob/784be60842d088596d7af938f90c689083677dca/v2v/output_rhv_upload.ml#L191

As you say we do need to preserve the command line option, but it'll
be ignored.
> > [...]
> >
> > > -# 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 UnixHTTPConnection(HTTPConnection):
> >
> > Why drop this part?
> 
> Not used now, imageio client includes this class:
>
https://github.com/oVirt/ovirt-imageio/blob/24c59f2e0ace784d9c993f6044475bb370058e70/daemon/ovirt_imageio/_internal/backends/http.py#L610
Good thing.

Thanks,

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
Fedora Windows cross-compiler. Compile Windows programs, test, and
build Windows installers. Over 100 libraries supported.
http://fedoraproject.org/wiki/MinGW

On Thu, Jul 9, 2020 at 4:12 PM Richard W.M. Jones <rjones@redhat.com>
wrote:
>
> On Thu, Jul 09, 2020 at 03:30:22PM +0300, Nir Soffer wrote:
> > Testing with a real server is easy. I have incomplete patch using
> > imageio server with some manual setup.
> [...]
>
> For a certain definition of "easy" :-)
>
> Our QE team tests -o rhv-upload from time to time, and will do an
> end-to-end test of the system when RHV 4.4 arrives with the new
> feature.
>
> For “make check” we need something that will run in a few seconds and
> doesn't have any major dependencies.
>
> > If we want to test with a fake, we can create fake client - no need
for runing
> > http sever:
> >
> > class FakeImageioClient:
> >
> >     def write(self, offset, buf):
> >         pass
> >
> >     ...
> >
> > This is easy, but we don't test much.
>
> Right, this is what I was thinking of for “make check”.  While it's
> not a comprehensive test, it is still testing something valuable:
> whether the Python code can compile and run from beginning to end
> without runtime errors.
So I'll create a fake client, and document how to test with a real server.

We can later try to do something smarter, like run the real server only if
you made it available, or added some environment variable.
> > I don't think this is the right approach. We already failed last
time when
> > we added the fake http server  the tests passed but the change to
support
> > them broke the real code.
> >
> > Wrong tests are the worst.
>
> Agreed.
>
> > > > - Need to require ovirt-imageio-client, providing the client
library.
> > >
> > > That's a simple change in virt-v2v packaging.  I don't
see this
> > > package in Fedora Koji.
> >
> > Is this an issue? oVirt is not really supported on Fedora these days.
>
> No it's not an issue, but the feature will not work on Fedora.  A bit
> surprised though - wouldn't you want people to be able to upload disk
> images to oVirt from their Fedora clients?
Sure I do, this is a major issue with this change.

Currently imageio is available via:
copr: https://copr.fedorainfracloud.org/coprs/nsoffer/ovirt-imageio-preview/
    fedora, centos, centos-stream
ovit repositories: https://resources.ovirt.org/pub/ovirt-4.4/
     el8/x86_64/ppc64le

I'll try to get this into Fedora.
> ...
> > > In RHEL I can see the package and the
> > > dependencies look quite light, basically just Python and
python3-six.
> > > Why is it only available for x86_64 and ppc64le?
> >
> > Probably because RHV is not built for other arches.
> >
> > Is this an issue? e.g. do we support importing vms in virt-v2v runing
on
> > arm, importing to oVirt/RHV running on x86/ppc?
>
> Virt-v2v is only built for x86-64 in RHEL 8, so it's not really an
issue.
>
> I was more wondering if there was some problem that prevented it from
> working on the other architectures thinking about the Fedora case, but
> if we're not going to package this up in Fedora it doesn't matter.
We never tried to build on other arches, but it should work. We have
few lines of C but they should be portable.
https://github.com/oVirt/ovirt-imageio/blob/master/daemon/ovirt_imageio/_internal/ioutil.c

I added aarch64 builds in copr, lets see how it goes:
https://copr.fedorainfracloud.org/coprs/nsoffer/ovirt-imageio-preview/build/1541540/
> > > > - params['rhv_direct'] is ignored, we always try
direct transfer now.
> > >
> > > We should drop it from the OCaml code?
> >
> > And break users that used this flag?
>
> Actually I meant just drop the params['rhv_direct'] field, ie:
>
https://github.com/libguestfs/virt-v2v/blob/784be60842d088596d7af938f90c689083677dca/v2v/output_rhv_upload.ml#L191
>
> As you say we do need to preserve the command line option, but it'll
> be ignored.
Sounds good.
> > > [...]
> > >
> > > > -# 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 UnixHTTPConnection(HTTPConnection):
> > >
> > > Why drop this part?
> >
> > Not used now, imageio client includes this class:
> >
https://github.com/oVirt/ovirt-imageio/blob/24c59f2e0ace784d9c993f6044475bb370058e70/daemon/ovirt_imageio/_internal/backends/http.py#L610
>
> Good thing.
>
> Thanks,
>
> Rich.
>
> --
> Richard Jones, Virtualization Group, Red Hat
http://people.redhat.com/~rjones
> Read my programming and virtualization blog: http://rwmj.wordpress.com
> Fedora Windows cross-compiler. Compile Windows programs, test, and
> build Windows installers. Over 100 libraries supported.
> http://fedoraproject.org/wiki/MinGW
>