Libguestfs - Oct 2019 - Re: [PATCH] v2v: Output saved overlays in a machine-readable fashion

On Fri, Oct 11, 2019 at 01:29:53PM +0100, Richard W.M. Jones
wrote:
>Thanks for explaining how this was going to work on IRC.
>
>Stepping back I think the problem is we're shoehorning features into a
>tool (virt-v2v) which was designed to do cold conversions, and was
>never meant to do either warm or in-place conversions.  The tool was
>hacked a while back for in-place but that mode is very awkward to use
>and we have never enabled it in RHEL for good reason.
>
>What would a tool which reused virt-v2v code and did what we really
>want look like?  The basic flow of objects in existing v2v/v2v.ml is:
>
Ideally there would be distinction between metadata and disks, so that I could
specify separately for disks and metadata where do they come from and where they
should go.  That way we could do things like "convert metadata from source
to
destination, but take disk data from the destination already and keep them
there" and other things like that.
>* let input, output = parse_cmdline ()
>
>    The -i and -o options from the command line are parsed, and
>    "input" and "output" objects constructed.
>
>* let source = open_source input
>
>    The input metadata is read (eg. libvirt XML, VMware VMX) and an
>    internal "source" object is created which models this
metadata,
>    plus (almost as a side-effect) links to the disks.  In the
>    following discussion remember that "source" ==
"metadata".
>
>* let overlays = create_overlays source.s_disks in
>  let g = Guestfs.guestfs () in
>  populate_overlays g overlays
>
>    The overlays are created and added as disks to a guestfs handle
>
>* let inspect = Inspect_source.inspect_source g in
>  let guestcaps = do_convert g inspect output in
>
>    This does inspection + conversion.  (The real do_convert function
>    takes the source struct as parameter, but it is not really used.)
>
>* let targets = output#prepare_targets overlays in
>  copy_targets targets input output
>
>    This creates the target disks and copies them.  (The real
>    output#prepare_targets method takes the source struct as a
>    parameter, but the actual objects only use the source.s_name and
>    .s_hypervisor fields).
>
>* output#create_metadata source targets guestcaps inspect
>
>    This creates the target metadata.
>
>What you want -- copy done elsewhere, convert in place, create metadata --
>is a very different flow.  It could look something like this:
>
>* let overlays = create_overlays source.s_disks in
>  let g = Guestfs.guestfs () in
>  populate_overlays g overlays
>  let inspect = Inspect_source.inspect_source g in
>  let guestcaps = do_convert g inspect output in
>  let targets = output#prepare_targets overlays in
>  for each overlay: qemu-img commit it
>  output#create_metadata source targets guestcaps inspect
>
From the calling application point of view the only difference here is that
virt-v2v calls qemu-img commit instead of qemu-img convert.  It is essentially
what I would like to have, but what you are describing above only fits my use
case and is not generally usable for upstream (at least how I see it, I
don't
know how else it is used).
>We need the source (ie. metadata) struct in the last step in order to
>create the metadata, so that still needs to be read from somewhere,
>and maybe -i libvirtxml is as good a place as any.
>
If metadata and disks have separate input+output settings (which is an overkill
in my opinion, at least in the current state) then this can come from the source
while disks are coming from the destination already.  But getting the xml and
modifying it is not *that* big of a deal (until we find problems there).
>I was going to come to a proper conclusion here, but I'm not yet sure
>what it is at the moment.  I'll think about this a bit more.  If you
>have any comments I'd like to hear them.
>
Purely from the usage point of view it would be the nicest thing if virt-v2v did
not do so many things at once.  I mean keep the functionality as is, but expose
the separate steps so that they do not need to be duplicated.  It would mean
exposing the guest disk conversion as an API or a CLI tool, converting metadata
between generic and specific formats as another one.  Generic format would be
for example similar to what -o json generates, keeping in mind that it would
have to have *all* the data that virt-v2v wants to use.  Then the composition of
all the steps in different order and/or with changed input data would be way
nicer.  Well, I think it would, but it might also not be the case.
>However there are some action items:
>
>(1) do_convert does not need the source parameter.  I think it could
>just be passed the .s_name field instead.
>
>(2) output#prepare_targets doesn't really need the source struct, but
>could probably get away with being passed just the .s_name field and
>maybe .s_hypervisor.
>
>(3) source.s_disks and the rest of the source struct seem to have
>sufficiently different usage that we can consider separating them.
>
>These changes would reduce coupling between stages.
>
To be honest I do not know what you are trying to do here.  If that helps
getting supported version of --debug-overlays, then great.  But to make it
absolutely clear, what --debug-overlays is so close to what I wanted the whole
time that the only better thing than that would be --commit-overlays and the
only thing that it would help with would be that I don't have to run one
easy command per disk.  Given the only "problem" with --debug-overlays
I can think of is the fact that it is marked as unsupported.  Other than that,
I'm very happy with this patch.  And given the fact that it does not need
scary code changes it also feels safe to use.
>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

On Fri, Oct 11, 2019 at 04:17:44PM +0200, Martin Kletzander
wrote:
> On Fri, Oct 11, 2019 at 01:29:53PM +0100, Richard W.M. Jones wrote:
> >What would a tool which reused virt-v2v code and did what we really
> >want look like?  The basic flow of objects in existing v2v/v2v.ml is:
>
> Ideally there would be distinction between metadata and disks, so
> that I could specify separately for disks and metadata where do they
> come from and where they should go.  That way we could do things
> like "convert metadata from source to destination, but take disk
> data from the destination already and keep them there" and other
> things like that.
Just want to point out that this particular operation is not possible
since target metadata depends on the result of conversion, and it's
not really possible to intuit it by looking at the converted disk.
> >However there are some action items:
> >
> >(1) do_convert does not need the source parameter.  I think it could
> >just be passed the .s_name field instead.
> >
> >(2) output#prepare_targets doesn't really need the source struct,
but
> >could probably get away with being passed just the .s_name field and
> >maybe .s_hypervisor.
> >
> >(3) source.s_disks and the rest of the source struct seem to have
> >sufficiently different usage that we can consider separating them.
> >
> >These changes would reduce coupling between stages.
> >
> To be honest I do not know what you are trying to do here.
These changes are just to simplify the code and make it easier to do
othe changes in future.
> If that helps getting supported version of --debug-overlays, then
> great.  But to make it absolutely clear, what --debug-overlays is so
> close to what I wanted the whole time that the only better thing
> than that would be --commit-overlays and the only thing that it
> would help with would be that I don't have to run one easy command
> per disk.  Given the only "problem" with --debug-overlays I can
> think of is the fact that it is marked as unsupported.  Other than
> that, I'm very happy with this patch.  And given the fact that it
> does not need scary code changes it also feels safe to use.
I'm trying to make something supportable from the point of view of
upstream.  We can't get into a situation where a major user of the
tool relies on debugging output and (as inevitably happens) we never
get a chance to fix it.  Overlays are entirely an internal detail of
how virt-v2v happens to work currently.

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

On Fri, Oct 11, 2019 at 03:48:43PM +0100, Richard W.M. Jones
wrote:
>On Fri, Oct 11, 2019 at 04:17:44PM +0200, Martin Kletzander wrote:
>> On Fri, Oct 11, 2019 at 01:29:53PM +0100, Richard W.M. Jones wrote:
>> >What would a tool which reused virt-v2v code and did what we really
>> >want look like?  The basic flow of objects in existing v2v/v2v.ml
is:
>>
>> Ideally there would be distinction between metadata and disks, so
>> that I could specify separately for disks and metadata where do they
>> come from and where they should go.  That way we could do things
>> like "convert metadata from source to destination, but take disk
>> data from the destination already and keep them there" and other
>> things like that.
>
>Just want to point out that this particular operation is not possible
>since target metadata depends on the result of conversion, and it's
>not really possible to intuit it by looking at the converted disk.
>
>> >However there are some action items:
>> >
>> >(1) do_convert does not need the source parameter.  I think it
could
>> >just be passed the .s_name field instead.
>> >
>> >(2) output#prepare_targets doesn't really need the source
struct, but
>> >could probably get away with being passed just the .s_name field
and
>> >maybe .s_hypervisor.
>> >
>> >(3) source.s_disks and the rest of the source struct seem to have
>> >sufficiently different usage that we can consider separating them.
>> >
>> >These changes would reduce coupling between stages.
>> >
>
>> To be honest I do not know what you are trying to do here.
>
>These changes are just to simplify the code and make it easier to do
>othe changes in future.
>
>> If that helps getting supported version of --debug-overlays, then
>> great.  But to make it absolutely clear, what --debug-overlays is so
>> close to what I wanted the whole time that the only better thing
>> than that would be --commit-overlays and the only thing that it
>> would help with would be that I don't have to run one easy command
>> per disk.  Given the only "problem" with --debug-overlays I
can
>> think of is the fact that it is marked as unsupported.  Other than
>> that, I'm very happy with this patch.  And given the fact that it
>> does not need scary code changes it also feels safe to use.
>
>I'm trying to make something supportable from the point of view of
>upstream.  We can't get into a situation where a major user of the
>tool relies on debugging output and (as inevitably happens) we never
>get a chance to fix it.  Overlays are entirely an internal detail of
>how virt-v2v happens to work currently.
>
OK, that makes sense.  It helps with understanding your e-mail =) In that case
yes, integrating `qemu-img commit` into the process is better.  I might go
through your e-mail again to make sure I didn't miss anything.

Have a nice weekend.
>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