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

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
>

On Thu, Jul 9, 2020 at 4:49 PM Nir Soffer <nsoffer@redhat.com>
wrote:
>
> 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
This includes now also aarch64:
https://copr.fedorainfracloud.org/coprs/nsoffer/ovirt-imageio-preview/build/1541540/

Do you think this is good enough for introducing this change upstream?
Having to enable copr repo to import vms from Fedora machine?
> 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
> >

On Thu, Jul 09, 2020 at 05:03:47PM +0300, Nir Soffer
wrote:
> On Thu, Jul 9, 2020 at 4:49 PM Nir Soffer <nsoffer@redhat.com> wrote:
> > > 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
> 
> This includes now also aarch64:
>
https://copr.fedorainfracloud.org/coprs/nsoffer/ovirt-imageio-preview/build/1541540/
> 
> Do you think this is good enough for introducing this change upstream?
> Having to enable copr repo to import vms from Fedora machine?
> 
> > ovit repositories: https://resources.ovirt.org/pub/ovirt-4.4/
> >      el8/x86_64/ppc64le
> >
> > I'll try to get this into Fedora.
All seems good to me.  If you want me to review a Fedora
packaging request then let me know.

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