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

If this information is useful, please help other people find it:
Share via:

Martin Kletzander

2018-Nov-23 13:10 UTC

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

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.

Also I forgot to format the patch with notes, so attaching them now, just for
completeness:

    This is basically just another occurrence of the osinfo "guessing"
that already
    exists in the inspector code.  If suggested, I can move it there and join
those
    together.
    
    I tried this on few disks from virt-builder, but not on any Windows one. 
All
    worked nicely except openSuSE tumbleweed which met an error earlier than the
XML
    creation phase.
    
    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.
    
    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.
>> 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.

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.

Martin

Richard W.M. Jones

2018-Nov-23 13:16 UTC

head link

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

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".

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’?
> >>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

Martin Kletzander

2018-Nov-23 13:26 UTC

head link

Re: [Libguestfs] [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

Reasonably Related Threads

Search for more apparently analagous threads

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

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

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

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

Reasonably Related Threads