On Thu, Oct 10, 2019 at 03:33:25PM +0100, Richard W.M. Jones wrote: >On Wed, Oct 09, 2019 at 02:19:46PM +0200, Martin Kletzander wrote: >> Even though this option is not to be used according to the manual, it: >> >> a) still might be useful even for machine-readable logs >> >> b) should not break the machine-readable output > >I'm a bit confused what you're trying to do here. > When working on a conversion where the disk is already copied from the source hypervisor to the destination storage (due to various reasons, e.g. having faster access to one of the storages or being able to use the disk_sync script to prefetch some of the data before the machine gets shut down) I need to utilize virt-v2v for two specific purposes: 1) to create the output metadata and 2) to make the changes in the guest disks (which are already copied) but not for the third thing that virt-v2v does as well, which is copying the disks from source to destination. For case #1 I initially used virt-v2v --no-copy, which creates the metadata, but discards the changes made to the disks in the overlays and for #2 I used --in-place which changes the data on the source (for which I crafted a specific XML to be used with -i libvirtxml), but does not output any metadata for the destination. There are couple of issues I am facing with this: a) I need to run the conversion twice b) --in-place does not know about the destination (and it does not do the same thing as if it knew) c) It should only be done using --no-copy first and --in-place second just to make sure the modifications are not done twice (although this is just to stay on the safe side). It still felt like a massive hack to me and while I still held on to the idea of keeping the overlays around without actually copying the data anywhere I found the --debug-overlays option. It fits my use case perfectly because what I am doing now (with this patch) is: i) run virt-v2v --no-copy with the proper destination ii) take the overlays and run qemu-img commit on them iii) remove the overlays This way all of my concerns are solved and the only one that arises is the fact that --debug-overlays is documented as "... only used for debugging and may be removed in a future version." One interesting thing is that I only found that information in the man page after I wrote this patch and started using it. The reason for it was that I started actually writing something very similar to this and I stumbled upon the code that already does that (with --debug-overlays). I also thought that if someone runs virt-v2v --machine-readable --debug-overlays that the machine readable output would be broken because the output is not json, but it looks like (I'm just trying it now) --machine-readable does not write any data to stdout or stderr, even when specified as --machine-readable=stream:stdout (or stderr) for simple libvirt2libvirt migration. So point (b) from the commit message is probably invalid. Oh, actually, I don't see it being used for anything else than printing apabilities and estimates. I wonder where the json messages in machine readable logfile that virt-v2v-wrapper requests are coming from. One last thing is that currently any application that wants to run virt-v2v (virt-v2v-wrapper in my case) needs to parse the output to get any meaningful data from it. What I tried here was using a new type of message called 'data' that upper applications could just parse and use. In the long run I could add more data to the output so that it is easier to use from virt-v2v-wrapper and other applications. But that is not something that is necessary for this particular patch (or the discussion about the --debug-overlays being supported in the long term), it just made it easier. Does that make sense? For some more context/info, see How I used it in a PoC here: https://github.com/nertpinx/v2v-conversion-host/blob/asdf/wrapper/log_par...

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:

* 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

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.

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.

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.

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

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

On Tue, Oct 15, 2019 at 04:53:03PM +0000, Roman Kagan wrote: > On Fri, Oct 11, 2019 at 01:29:53PM +0100, Richard W.M. Jones wrote: > > 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. > > As I'm guilty for introducing --in-place, let me chime in with my 2c > and humbly disagree with it being awkward to use. I think the only > thing that's awkward is its tight integration into virt-v2v; it'd > make more sense being a separate tool whose sole purpose would be to > tune the guest to be bootable/operational in the provided > configuration. My original comments there are rather rude. What I should have said is the use of the input metadata (eg. --inplace -i libvirtxml) as a kind of configuration file for selecting the rcaps (eg. whether to install virtio drivers) are somewhat non-obvious, although as you say in the context of virt-v2v that's all that can be done. I would - same as you - like to split out virt-v2v much more, into more streamlined tools (but without breaking backwards compatibility or removing features). Still don't know how, but I'll read the rest of your email in more detail as part of thinking about this.