Libguestfs - Nov 2018 - Re: [PATCH] v2v: Add support for libosinfo metadata

On Fri, Nov 23, 2018 at 01:16:55PM +0000, Richard W.M. Jones
wrote:
>On Fri, Nov 23, 2018 at 02:10:01PM +0100, Martin Kletzander wrote:
>> On Fri, Nov 23, 2018 at 11:53:05AM +0000, Richard W.M. Jones wrote:
>> >On Fri, Nov 23, 2018 at 12:39:44PM +0100, Martin Kletzander wrote:
>> >>There's a standardized libosinfo namespace for libvirt
domain metadata.  For now
>> >>it supports the id of the OS only.  However that is still a
very helpful feature
>> >>that is already supported in gnome-boxes and virt-manager (at
least).
>> >>
>> >>The discussion happened here:
>> >>
>> >> 
https://www.redhat.com/archives/libosinfo/2018-September/msg00003.html
>> >
>> >It's a shame I missed this thread because I'd probably have
asked why
>> >we're inventing yet another scheme for naming guests. 
Particularly in
>> >the libosinfo project which has already got the canonical naming
>> >scheme!
>> >
>> >Anyway that's water under the bridge now so ...
>> >
>>
>> But it is using the osinfo naming scheme.  I mean the ID.
>
>Not sure I understand.  osinfo currently has osinfo IDs like say
>"rhel7.6", but now we've invented a new scheme
>"http://redhat.com/rhel/7.6".
>
It's not new.  Libosinfo has support for both.  The first one,
"rhel7.6" in this
case, is a "short id", where the second one is actually called an
"ID".

Example output of `osinfo-query -f short-id,id,name os short-id=rhel7.4`
(truncated) should explain:

  rhel7.4 | Red Hat Enterprise Linux 7.4 | http://redhat.com/rhel/7.4
>In any case this isn't the place to argue about the merits of the new
>scheme, since it seems to have been settled in libosinfo and libvirt.
>
>>    The matching code could be made shorter, but since this is my
>>    "tryout patch" for libguestfs, I'm just glad it works.
I'd
>>    appreciate literally *any* comment to any part of this.
>
>Apart from the duplicate case (which is in fact not duplicated), I
>didn't have any other problem.  However ...
>
>>    I haven't find any unit tests for these kind of functions, so no
>>    tests are added.  If there is a place where tests would fit
>>    nicely, feel free to let me know.
>
>... I think the tests might actually be broken by this patch.  Did you
>try: ‘make -C test-data check && make -C v2v check’?
>
good point, probably not after the patch. will do.
>> >>So let's add the support to local and libvirt outputs.
>> >>
>> >>Signed-off-by: Martin Kletzander <mkletzan@redhat.com>
>> >>---
>> >> v2v/create_libvirt_xml.ml  | 109
++++++++++++++++++++++++++++++++++++-
>> >> v2v/create_libvirt_xml.mli |   1 +
>> >> v2v/output_libvirt.ml      |   4 +-
>> >> v2v/output_local.ml        |   4 +-
>> >> 4 files changed, 113 insertions(+), 5 deletions(-)
>> >>
>> >>diff --git a/v2v/create_libvirt_xml.ml
b/v2v/create_libvirt_xml.ml
>> >>index 55e83e8bc1b9..180f3768792b 100644
>> >>--- a/v2v/create_libvirt_xml.ml
>> >>+++ b/v2v/create_libvirt_xml.ml
>> >>@@ -34,8 +34,102 @@ let find_target_disk targets { s_disk_id =
id } >> >>   try List.find (fun t ->
t.target_overlay.ov_source.s_disk_id = id) targets
>> >>   with Not_found -> assert false
>> >>
>> >>+let get_osinfo_id = function
>> >>+  | { i_type = "linux"; i_distro = "rhel";
>> >>+      i_major_version = major; i_minor_version = minor } ->
>> >>+    Some (sprintf "http://redhat.com/rhel/%d.%d"
major minor)
>> >>+
>> >>+  | { i_type = "linux"; i_distro =
"centos";
>> >>+      i_major_version = major; i_minor_version = minor } when
major < 7 ->
>> >>+    Some (sprintf "http://centos.org/centos/%d.%d"
major minor)
>> >>+
>> >>+  | { i_type = "linux"; i_distro =
"centos"; i_major_version = major } ->
>> >>+    Some (sprintf "http://centos.org/centos/%d.0"
major)
>> >>+
>> >>+  | { i_type = "linux"; i_distro = "sles";
>> >>+      i_major_version = major; i_minor_version = 0 } ->
>> >>+    Some (sprintf "http://suse.com/sles/%d" major)
>> >>+
>> >>+  | { i_type = "linux"; i_distro = "sles";
>> >>+      i_major_version = major; i_minor_version = minor } ->
>> >>+    Some (sprintf "http://suse.com/sles/%d.%d" major
minor)
>> >
>> >SLES is duplicated?
>> >
>>
>> Oh, I removed the comment i had there.  osinfo does not use 0 in the
version,
>> it's just SLES 10, SLES 10.1 (for SP1), 10.2 (for SP2) and so on.
>
>Oh I see now, it's not duplicated because of the check for
>i_minor_version = 0.
>
>> But I just noticed I forgot to differentiate SLED from SLES.  It
>> should be possible based on i_product_name.  I think it would make
>> sense for libosinfo and it doesn't look like they are separate in
>> libguestfs (on purpose).  Would it also make sense to separate them
>> here?  Is it even possible?  I'm not sure what the compatibility
>> promises are.
>
>No idea.  We just try to provide libvirt XML which is as useful as
>possible.
>
>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

On Fri, Nov 23, 2018 at 02:26:08PM +0100, Martin Kletzander
wrote:
>On Fri, Nov 23, 2018 at 01:16:55PM +0000, Richard W.M. Jones wrote:
>>On Fri, Nov 23, 2018 at 02:10:01PM +0100, Martin Kletzander wrote:
[...]
>>>    I haven't find any unit tests for these kind of functions,
so no
>>>    tests are added.  If there is a place where tests would fit
>>>    nicely, feel free to let me know.
>>
>>... I think the tests might actually be broken by this patch.  Did you
>>try: ‘make -C test-data check && make -C v2v check’?
>>
>
>good point, probably not after the patch. will do.
>
The tests pass.  Should I add/change something in there as well since I'm
going
to be sending v2 anyway?

[...]
>>> But I just noticed I forgot to differentiate SLED from SLES.  It
>>> should be possible based on i_product_name.  I think it would make
>>> sense for libosinfo and it doesn't look like they are separate
in
>>> libguestfs (on purpose).  Would it also make sense to separate them
>>> here?  Is it even possible?  I'm not sure what the
compatibility
>>> promises are.
>>
>>No idea.  We just try to provide libvirt XML which is as useful as
>>possible.
>>
I added SLED/SLES differentiation to v2.

On Fri, Nov 23, 2018 at 03:55:15PM +0100, Martin Kletzander
wrote:
>On Fri, Nov 23, 2018 at 02:26:08PM +0100, Martin Kletzander wrote:
>>On Fri, Nov 23, 2018 at 01:16:55PM +0000, Richard W.M. Jones wrote:
>>>On Fri, Nov 23, 2018 at 02:10:01PM +0100, Martin Kletzander wrote:
>
>[...]
>
>>>>    I haven't find any unit tests for these kind of
functions, so no
>>>>    tests are added.  If there is a place where tests would fit
>>>>    nicely, feel free to let me know.
>>>
>>>... I think the tests might actually be broken by this patch.  Did
you
>>>try: ‘make -C test-data check && make -C v2v check’?
>>>
>>
>>good point, probably not after the patch. will do.
>>
>
>The tests pass.  Should I add/change something in there as well since
I'm going
>to be sending v2 anyway?
>
My bad, I had the appliance and daemon disabled.  I'll build with that and
update the patch accordingly, sorry for the noise.
>[...]
>
>>>> But I just noticed I forgot to differentiate SLED from SLES. 
It
>>>> should be possible based on i_product_name.  I think it would
make
>>>> sense for libosinfo and it doesn't look like they are
separate in
>>>> libguestfs (on purpose).  Would it also make sense to separate
them
>>>> here?  Is it even possible?  I'm not sure what the
compatibility
>>>> promises are.
>>>
>>>No idea.  We just try to provide libvirt XML which is as useful as
>>>possible.
>>>
>
>I added SLED/SLES differentiation to v2.

On Fri, Nov 23, 2018 at 03:55:15PM +0100, Martin Kletzander
wrote:
> On Fri, Nov 23, 2018 at 02:26:08PM +0100, Martin Kletzander wrote:
> >On Fri, Nov 23, 2018 at 01:16:55PM +0000, Richard W.M. Jones wrote:
> >>On Fri, Nov 23, 2018 at 02:10:01PM +0100, Martin Kletzander wrote:
> 
> [...]
> 
> >>>   I haven't find any unit tests for these kind of
functions, so no
> >>>   tests are added.  If there is a place where tests would fit
> >>>   nicely, feel free to let me know.
> >>
> >>... I think the tests might actually be broken by this patch.  Did
you
> >>try: ‘make -C test-data check && make -C v2v check’?
> >>
> >
> >good point, probably not after the patch. will do.
> >
> 
> The tests pass.  Should I add/change something in there as well since
I'm going
> to be sending v2 anyway?
I see that the test I was expecting might fail (test-v2v-o-libvirt.sh)
in fact only uploads the XML to libvirt without checking it in detail,
and we don't have a test for -o local at all.

Nevermind.  Just post patch v2.

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