Libguestfs - Jul 2015 - Re: [PATCH] v2v: add --in-place mode

In this mode, converting of the VM configuration, setting up the
rollback path for error cases, transforming the VM storage and so on is
taken care of by a third-party toolset, and virt-v2v is only supposed to
tune up the guest OS directly inside the source VM, to enable it to boot
and run under the input hypervisor.

Signed-off-by: Roman Kagan <rkagan@virtuozzo.com>
---
 tests/guests/guests.xml.in |  16 +++++++
 v2v/Makefile.am            |   1 +
 v2v/cmdline.ml             |   7 +++-
 v2v/test-v2v-in-place.sh   |  81 +++++++++++++++++++++++++++++++++++
 v2v/v2v.ml                 | 102 +++++++++++++++++++++++++++------------------
 v2v/virt-v2v.pod           |  17 ++++++++
 6 files changed, 183 insertions(+), 41 deletions(-)
 create mode 100755 v2v/test-v2v-in-place.sh

diff --git a/tests/guests/guests.xml.in b/tests/guests/guests.xml.in
index 8f7ac81..6f08b80 100644
--- a/tests/guests/guests.xml.in
+++ b/tests/guests/guests.xml.in
@@ -279,4 +279,20 @@
     </devices>
   </domain>
 
+  <domain type='test'>
+    <name>windows-overlay</name>
+    <memory>1048576</memory>
+    <os>
+      <type>hvm</type>
+      <boot dev='hd'/>
+    </os>
+    <devices>
+      <disk type='file' device='disk'>
+        <driver name='qemu' type='qcow2'/>
+        <source file='@abs_builddir@/windows-overlay.qcow2'/>
+        <target dev='vda' bus='virtio'/>
+      </disk>
+    </devices>
+  </domain>
+
 </node>
diff --git a/v2v/Makefile.am b/v2v/Makefile.am
index 06da002..dae063c 100644
--- a/v2v/Makefile.am
+++ b/v2v/Makefile.am
@@ -232,6 +232,7 @@ TESTS += \
 	test-v2v-cdrom.sh \
 	test-v2v-i-ova.sh \
 	test-v2v-i-disk.sh \
+	test-v2v-in-place.sh \
 	test-v2v-machine-readable.sh \
 	test-v2v-networks-and-bridges.sh \
 	test-v2v-no-copy.sh \
diff --git a/v2v/cmdline.ml b/v2v/cmdline.ml
index eaf57dc..2a3224a 100644
--- a/v2v/cmdline.ml
+++ b/v2v/cmdline.ml
@@ -37,6 +37,7 @@ let parse_cmdline ()    let output_format = ref ""
in
   let output_name = ref "" in
   let output_storage = ref "" in
+  let in_place = ref false in
   let password_file = ref "" in
   let print_source = ref false in
   let qemu_boot = ref false in
@@ -160,6 +161,7 @@ let parse_cmdline ()      "-of",      
Arg.Set_string output_format, "raw|qcow2 " ^ s_"Set output
format";
     "-on",       Arg.Set_string output_name, "name " ^
s_"Rename guest when converting";
     "-os",       Arg.Set_string output_storage, "storage "
^ s_"Set output storage location";
+    "--in-place", Arg.Set in_place,         " " ^
s_"Only tune the guest in the input VM";
     "--password-file", Arg.Set_string password_file, "file
" ^ s_"Use password from file";
     "--print-source", Arg.Set print_source, " " ^
s_"Print source and stop";
     "--qemu-boot", Arg.Set qemu_boot,       " " ^
s_"Boot in qemu (-o qemu only)";
@@ -226,6 +228,7 @@ read the man page virt-v2v(1).
   let output_mode = !output_mode in
   let output_name = match !output_name with "" -> None | s ->
Some s in
   let output_storage = !output_storage in
+  let in_place = !in_place in
   let password_file = match !password_file with "" -> None | s
-> Some s in
   let print_source = !print_source in
   let qemu_boot = !qemu_boot in
@@ -305,6 +308,8 @@ read the man page virt-v2v(1).
       Input_ova.input_ova filename in
 
   (* Parse the output mode. *)
+  if output_mode <> `Not_set && in_place then
+      error (f_"-o and --in-place cannot be used at the same time");
   let output      match output_mode with
     | `Glance ->
@@ -386,5 +391,5 @@ read the man page virt-v2v(1).
 
   input, output,
   debug_gc, debug_overlays, do_copy, network_map, no_trim,
-  output_alloc, output_format, output_name,
+  output_alloc, output_format, output_name, in_place,
   print_source, root_choice
diff --git a/v2v/test-v2v-in-place.sh b/v2v/test-v2v-in-place.sh
new file mode 100755
index 0000000..c19707e
--- /dev/null
+++ b/v2v/test-v2v-in-place.sh
@@ -0,0 +1,81 @@
+#!/bin/bash -
+# libguestfs virt-v2v test script
+# Copyright (C) 2014 Red Hat Inc.
+# Copyright (C) 2015 Parallels IP Holdings GmbH.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program; if not, write to the Free Software
+# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
+
+# Test --in-place.
+
+unset CDPATH
+export LANG=C
+set -e
+
+if [ -n "$SKIP_TEST_V2V_IN_PLACE_SH" ]; then
+    echo "$0: test skipped because environment variable is set"
+    exit 77
+fi
+
+if [ "$(guestfish get-backend)" = "uml" ]; then
+    echo "$0: test skipped because UML backend does not support
network"
+    exit 77
+fi
+
+# You shouldn't be running the tests as root anyway, but in this case
+# it's especially bad because we don't want to start creating guests
+# or storage pools in the system namespace.
+if [ "$(id -u)" -eq 0 ]; then
+    echo "$0: test skipped because you're running tests as root. 
Don't do that!"
+    exit 77
+fi
+
+guests_dir="$(cd $(dirname $0)/../tests/guests && pwd)"
+libvirt_uri="test://$guests_dir/guests.xml"
+
+f="$guests_dir/windows.img"
+if ! test -f $f || ! test -s $f; then
+    echo "$0: test skipped because phony Windows image was not
created"
+    exit 77
+fi
+
+virt_tools_data_dir=${VIRT_TOOLS_DATA_DIR:-/usr/share/virt-tools}
+if ! test -r $virt_tools_data_dir/rhsrvany.exe; then
+    echo "$0: test skipped because rhsrvany.exe is not installed"
+    exit 77
+fi
+
+fo="$guests_dir/windows-overlay.qcow2"
+rm -f $fo
+qemu-img create -f qcow2 -b $f -o compat=1.1,backing_fmt=raw $fo
+md5="$(md5sum $f)"
+
+$VG virt-v2v --debug-gc \
+    -i libvirt -ic "$libvirt_uri" windows-overlay \
+    --in-place
+
+# Test some aspects of the target disk image.
+guestfish --ro -a $fo -i <<EOF
+  is-dir "/Program Files/Red Hat/Firstboot"
+  is-file "/Program Files/Red Hat/Firstboot/firstboot.bat"
+  is-dir "/Program Files/Red Hat/Firstboot/scripts"
+  is-dir "/Windows/Drivers/VirtIO"
+EOF
+
+
+# Test the base image remained untouched
+test "$md5" = "$(md5sum $f)"
+
+# Clean up.
+rm -r $fo
diff --git a/v2v/v2v.ml b/v2v/v2v.ml
index 242f129..b744056 100644
--- a/v2v/v2v.ml
+++ b/v2v/v2v.ml
@@ -47,7 +47,8 @@ let rec main ()    (* Handle the command line. *)
   let input, output,
     debug_gc, debug_overlays, do_copy, network_map, no_trim,
-    output_alloc, output_format, output_name, print_source, root_choice +   
output_alloc, output_format, output_name, in_place, print_source,
+    root_choice      Cmdline.parse_cmdline () in
 
   (* Print the version, easier than asking users to tell us. *)
@@ -117,52 +118,70 @@ let rec main ()      ) nics in
     { source with s_nics = nics } in
 
-  (* Create a qcow2 v3 overlay to protect the source image(s).  There
-   * is a specific reason to use the newer qcow2 variant: Because the
-   * L2 table can store zero clusters efficiently, and because
-   * discarded blocks are stored as zero clusters, this should allow us
-   * to fstrim/blkdiscard and avoid copying significant parts of the
-   * data over the wire.
-   *)
-  message (f_"Creating an overlay to protect the source from being
modified");
   let overlay_dir = (new Guestfs.guestfs ())#get_cachedir () in
-  let overlays -    List.map (
-      fun ({ s_qemu_uri = qemu_uri; s_format = format } as source) ->
-        let overlay_file -          Filename.temp_file ~temp_dir:overlay_dir
"v2vovl" ".qcow2" in
-        unlink_on_exit overlay_file;
-
-        let options -          "compat=1.1" ^
-            (match format with None -> ""
-            | Some fmt -> ",backing_fmt=" ^ fmt) in
-        let cmd -          sprintf "qemu-img create -q -f qcow2 -b %s -o
%s %s"
-            (quote qemu_uri) (quote options) overlay_file in
-        if verbose () then printf "%s\n%!" cmd;
-        if Sys.command cmd <> 0 then
-          error (f_"qemu-img command failed, see earlier errors");
-
-        (* Sanity check created overlay (see below). *)
-        if not ((new G.guestfs ())#disk_has_backing_file overlay_file) then
-          error (f_"internal error: qemu-img did not create overlay with
backing file");
-
-        overlay_file, source
-    ) source.s_disks in
+  let overlays = (
+    if not in_place then (
+      (* Create a qcow2 v3 overlay to protect the source image(s).  There
+       * is a specific reason to use the newer qcow2 variant: Because the
+       * L2 table can store zero clusters efficiently, and because
+       * discarded blocks are stored as zero clusters, this should allow us
+       * to fstrim/blkdiscard and avoid copying significant parts of the
+       * data over the wire.
+       *)
+      message (f_"Creating an overlay to protect the source from being
modified");
+      List.map (
+        fun ({ s_qemu_uri = qemu_uri; s_format = format } as source) ->
+          let overlay_file +            Filename.temp_file
~temp_dir:overlay_dir "v2vovl" ".qcow2" in
+          unlink_on_exit overlay_file;
+
+          let options +            "compat=1.1" ^
+              (match format with None -> ""
+              | Some fmt -> ",backing_fmt=" ^ fmt) in
+          let cmd +            sprintf "qemu-img create -q -f qcow2 -b %s
-o %s %s"
+              (quote qemu_uri) (quote options) overlay_file in
+          if verbose () then printf "%s\n%!" cmd;
+          if Sys.command cmd <> 0 then
+            error (f_"qemu-img command failed, see earlier errors");
+
+          (* Sanity check created overlay (see below). *)
+          if not ((new G.guestfs ())#disk_has_backing_file overlay_file) then
+            error (f_"internal error: qemu-img did not create overlay with
backing file");
+
+          overlay_file, source
+      ) source.s_disks
+    ) else []
+  ) in
 
   (* Open the guestfs handle. *)
-  message (f_"Opening the overlay");
+  if in_place then
+    message (f_"Opening the guest disks")
+  else
+    message (f_"Opening the overlay");
   let g = new G.guestfs () in
   if trace () then g#set_trace true;
   if verbose () then g#set_verbose true;
   g#set_network true;
-  List.iter (
-    fun (overlay_file, _) ->
-      g#add_drive_opts overlay_file
-        ~format:"qcow2" ~cachemode:"unsafe"
~discard:"besteffort"
-        ~copyonread:true
-  ) overlays;
+  if in_place then (
+    List.iter (
+      fun ({s_qemu_uri = qemu_uri; s_format = format}) ->
+        match format with
+        | None ->
+            g#add_drive_opts qemu_uri ~cachemode:"unsafe"
~discard:"besteffort"
+        | Some fmt ->
+            g#add_drive_opts qemu_uri ~format:fmt ~cachemode:"unsafe"
+              ~discard:"besteffort"
+    ) source.s_disks
+  ) else (
+    List.iter (
+      fun (overlay_file, _) ->
+        g#add_drive_opts overlay_file
+          ~format:"qcow2" ~cachemode:"unsafe"
~discard:"besteffort"
+          ~copyonread:true
+    ) overlays
+  );
 
   g#launch ();
 
@@ -294,6 +313,9 @@ let rec main ()    g#shutdown ();
   g#close ();
 
+  if in_place then
+    exit 0;
+
   (* Does the guest require UEFI on the target? *)
   message (f_"Checking if the guest needs BIOS or UEFI to boot");
   let target_firmware diff --git a/v2v/virt-v2v.pod b/v2v/virt-v2v.pod
index ef2dee1..eda1cf7 100644
--- a/v2v/virt-v2v.pod
+++ b/v2v/virt-v2v.pod
@@ -9,6 +9,8 @@ virt-v2v - Convert a guest to use KVM
  virt-v2v -ic vpx://vcenter.example.com/Datacenter/esxi vmware_guest \
    -o rhev -os rhev.nfs:/export_domain --network rhevm
 
+ virt-v2v -ic qemu:///system qemu_guest --in-place
+
  virt-v2v -i libvirtxml guest-domain.xml -o local -os /var/tmp
 
  virt-v2v -i disk disk.img -o local -os /var/tmp
@@ -75,6 +77,9 @@ booting the guest directly in qemu (mainly for testing).
 I<-o rhev> is used to write to a RHEV-M / oVirt target.  I<-o vdsm>
 is only used when virt-v2v runs under VDSM control.
 
+I<--in-place> instructs virt-v2v to customize the guest OS in the input
+virtual machine, instead of creating a new VM in the target hypervisor.
+
 =head1 EXAMPLES
 
 =head2 Convert from VMware vCenter server to local libvirt
@@ -518,6 +523,18 @@ C<root>.
 You will get an error if virt-v2v is unable to mount/write to the
 Export Storage Domain.
 
+=item B<--in-place>
+
+Do not create an output virtual machine in the target hypervisor.
+Instead, adjust the guest OS in the source VM to run in the input
+hypervisor.
+
+This mode is meant for integration with other toolsets, which take the
+responsibility of converting the VM configuration, providing for
+rollback in case of errors, transforming the storage, etc.
+
+Conflicts with all I<-o *> options.
+
 =item B<--password-file> file
 
 Instead of asking for password(s) interactively, pass the password
-- 
2.4.3

On Mon, Jul 27, 2015 at 07:29:57PM +0300, Roman Kagan
wrote:
> In this mode, converting of the VM configuration, setting up the
> rollback path for error cases, transforming the VM storage and so on is
> taken care of by a third-party toolset, and virt-v2v is only supposed to
> tune up the guest OS directly inside the source VM, to enable it to boot
> and run under the input hypervisor.
> 
> Signed-off-by: Roman Kagan <rkagan@virtuozzo.com>
> ---
>  tests/guests/guests.xml.in |  16 +++++++
>  v2v/Makefile.am            |   1 +
>  v2v/cmdline.ml             |   7 +++-
>  v2v/test-v2v-in-place.sh   |  81 +++++++++++++++++++++++++++++++++++
>  v2v/v2v.ml                 | 102
+++++++++++++++++++++++++++------------------
>  v2v/virt-v2v.pod           |  17 ++++++++
>  6 files changed, 183 insertions(+), 41 deletions(-)
>  create mode 100755 v2v/test-v2v-in-place.sh
> 
> diff --git a/tests/guests/guests.xml.in b/tests/guests/guests.xml.in
> index 8f7ac81..6f08b80 100644
> --- a/tests/guests/guests.xml.in
> +++ b/tests/guests/guests.xml.in
> @@ -279,4 +279,20 @@
>      </devices>
>    </domain>
>  
> +  <domain type='test'>
> +    <name>windows-overlay</name>
> +    <memory>1048576</memory>
> +    <os>
> +      <type>hvm</type>
> +      <boot dev='hd'/>
> +    </os>
> +    <devices>
> +      <disk type='file' device='disk'>
> +        <driver name='qemu' type='qcow2'/>
> +        <source
file='@abs_builddir@/windows-overlay.qcow2'/>
> +        <target dev='vda' bus='virtio'/>
> +      </disk>
> +    </devices>
> +  </domain>
Let's not add this to tests/guests, since this disk image is only used
for a single test in the v2v/ subdirectory.

You can change the new test in the v2v/ subdirectory so it creates (or
is supplied with) the XML.  There are various tests which work like
that already - eg: v2v/test-v2v-cdrom.*
> diff --git a/v2v/Makefile.am b/v2v/Makefile.am
> index 06da002..dae063c 100644
> --- a/v2v/Makefile.am
> +++ b/v2v/Makefile.am
> @@ -232,6 +232,7 @@ TESTS += \
>  	test-v2v-cdrom.sh \
>  	test-v2v-i-ova.sh \
>  	test-v2v-i-disk.sh \
> +	test-v2v-in-place.sh \
>  	test-v2v-machine-readable.sh \
>  	test-v2v-networks-and-bridges.sh \
>  	test-v2v-no-copy.sh \
> diff --git a/v2v/cmdline.ml b/v2v/cmdline.ml
> index eaf57dc..2a3224a 100644
> --- a/v2v/cmdline.ml
> +++ b/v2v/cmdline.ml
> @@ -37,6 +37,7 @@ let parse_cmdline () >    let output_format = ref
"" in
>    let output_name = ref "" in
>    let output_storage = ref "" in
> +  let in_place = ref false in
>    let password_file = ref "" in
>    let print_source = ref false in
>    let qemu_boot = ref false in
> @@ -160,6 +161,7 @@ let parse_cmdline () >      "-of",      
Arg.Set_string output_format, "raw|qcow2 " ^ s_"Set output
format";
>      "-on",       Arg.Set_string output_name, "name " ^
s_"Rename guest when converting";
>      "-os",       Arg.Set_string output_storage, "storage
" ^ s_"Set output storage location";
> +    "--in-place", Arg.Set in_place,         " " ^
s_"Only tune the guest in the input VM";
>      "--password-file", Arg.Set_string password_file, "file
" ^ s_"Use password from file";
>      "--print-source", Arg.Set print_source, " " ^
s_"Print source and stop";
>      "--qemu-boot", Arg.Set qemu_boot,       " " ^
s_"Boot in qemu (-o qemu only)";
> @@ -226,6 +228,7 @@ read the man page virt-v2v(1).
>    let output_mode = !output_mode in
>    let output_name = match !output_name with "" -> None | s
-> Some s in
>    let output_storage = !output_storage in
> +  let in_place = !in_place in
>    let password_file = match !password_file with "" -> None |
s -> Some s in
>    let print_source = !print_source in
>    let qemu_boot = !qemu_boot in
> @@ -305,6 +308,8 @@ read the man page virt-v2v(1).
>        Input_ova.input_ova filename in
>  
>    (* Parse the output mode. *)
> +  if output_mode <> `Not_set && in_place then
> +      error (f_"-o and --in-place cannot be used at the same
time");
>    let output >      match output_mode with
>      | `Glance ->
> @@ -386,5 +391,5 @@ read the man page virt-v2v(1).
>  
>    input, output,
>    debug_gc, debug_overlays, do_copy, network_map, no_trim,
> -  output_alloc, output_format, output_name,
> +  output_alloc, output_format, output_name, in_place,
>    print_source, root_choice
> diff --git a/v2v/test-v2v-in-place.sh b/v2v/test-v2v-in-place.sh
> new file mode 100755
> index 0000000..c19707e
> --- /dev/null
> +++ b/v2v/test-v2v-in-place.sh
> @@ -0,0 +1,81 @@
> +#!/bin/bash -
> +# libguestfs virt-v2v test script
> +# Copyright (C) 2014 Red Hat Inc.
> +# Copyright (C) 2015 Parallels IP Holdings GmbH.
> +#
> +# This program is free software; you can redistribute it and/or modify
> +# it under the terms of the GNU General Public License as published by
> +# the Free Software Foundation; either version 2 of the License, or
> +# (at your option) any later version.
> +#
> +# This program is distributed in the hope that it will be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +# GNU General Public License for more details.
> +#
> +# You should have received a copy of the GNU General Public License
> +# along with this program; if not, write to the Free Software
> +# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301
USA.
> +
> +# Test --in-place.
> +
> +unset CDPATH
> +export LANG=C
> +set -e
> +
> +if [ -n "$SKIP_TEST_V2V_IN_PLACE_SH" ]; then
> +    echo "$0: test skipped because environment variable is set"
> +    exit 77
> +fi
> +
> +if [ "$(guestfish get-backend)" = "uml" ]; then
> +    echo "$0: test skipped because UML backend does not support
network"
> +    exit 77
> +fi
> +
> +# You shouldn't be running the tests as root anyway, but in this case
> +# it's especially bad because we don't want to start creating
guests
> +# or storage pools in the system namespace.
> +if [ "$(id -u)" -eq 0 ]; then
> +    echo "$0: test skipped because you're running tests as root. 
Don't do that!"
> +    exit 77
> +fi
> +
> +guests_dir="$(cd $(dirname $0)/../tests/guests && pwd)"
> +libvirt_uri="test://$guests_dir/guests.xml"
> +
> +f="$guests_dir/windows.img"
> +if ! test -f $f || ! test -s $f; then
> +    echo "$0: test skipped because phony Windows image was not
created"
> +    exit 77
> +fi
> +
> +virt_tools_data_dir=${VIRT_TOOLS_DATA_DIR:-/usr/share/virt-tools}
> +if ! test -r $virt_tools_data_dir/rhsrvany.exe; then
> +    echo "$0: test skipped because rhsrvany.exe is not
installed"
> +    exit 77
> +fi
> +
> +fo="$guests_dir/windows-overlay.qcow2"
> +rm -f $fo
> +qemu-img create -f qcow2 -b $f -o compat=1.1,backing_fmt=raw $fo
> +md5="$(md5sum $f)"
> +
> +$VG virt-v2v --debug-gc \
> +    -i libvirt -ic "$libvirt_uri" windows-overlay \
> +    --in-place
> +
> +# Test some aspects of the target disk image.
> +guestfish --ro -a $fo -i <<EOF
> +  is-dir "/Program Files/Red Hat/Firstboot"
> +  is-file "/Program Files/Red Hat/Firstboot/firstboot.bat"
> +  is-dir "/Program Files/Red Hat/Firstboot/scripts"
> +  is-dir "/Windows/Drivers/VirtIO"
> +EOF
> +
> +
> +# Test the base image remained untouched
> +test "$md5" = "$(md5sum $f)"
> +
> +# Clean up.
> +rm -r $fo
> diff --git a/v2v/v2v.ml b/v2v/v2v.ml
> index 242f129..b744056 100644
> --- a/v2v/v2v.ml
> +++ b/v2v/v2v.ml
> @@ -47,7 +47,8 @@ let rec main () >    (* Handle the command line. *)
>    let input, output,
>      debug_gc, debug_overlays, do_copy, network_map, no_trim,
> -    output_alloc, output_format, output_name, print_source, root_choice
> +    output_alloc, output_format, output_name, in_place, print_source,
> +    root_choice >      Cmdline.parse_cmdline () in
>  
>    (* Print the version, easier than asking users to tell us. *)
> @@ -117,52 +118,70 @@ let rec main () >      ) nics in
>      { source with s_nics = nics } in
>  
> -  (* Create a qcow2 v3 overlay to protect the source image(s).  There
> -   * is a specific reason to use the newer qcow2 variant: Because the
> -   * L2 table can store zero clusters efficiently, and because
> -   * discarded blocks are stored as zero clusters, this should allow us
> -   * to fstrim/blkdiscard and avoid copying significant parts of the
> -   * data over the wire.
> -   *)
> -  message (f_"Creating an overlay to protect the source from being
modified");
>    let overlay_dir = (new Guestfs.guestfs ())#get_cachedir () in
Part of this patch is a refactoring -- moving the overlay_dir
assigning from within the loop up to here.  Put that into a separate
commit, as it's trivial to review on its own.
> -  let overlays > -    List.map (
> -      fun ({ s_qemu_uri = qemu_uri; s_format = format } as source) ->
> -        let overlay_file > -          Filename.temp_file
~temp_dir:overlay_dir "v2vovl" ".qcow2" in
> -        unlink_on_exit overlay_file;
> -
> -        let options > -          "compat=1.1" ^
> -            (match format with None -> ""
> -            | Some fmt -> ",backing_fmt=" ^ fmt) in
> -        let cmd > -          sprintf "qemu-img create -q -f qcow2
-b %s -o %s %s"
> -            (quote qemu_uri) (quote options) overlay_file in
> -        if verbose () then printf "%s\n%!" cmd;
> -        if Sys.command cmd <> 0 then
> -          error (f_"qemu-img command failed, see earlier
errors");
> -
> -        (* Sanity check created overlay (see below). *)
> -        if not ((new G.guestfs ())#disk_has_backing_file overlay_file)
then
> -          error (f_"internal error: qemu-img did not create overlay
with backing file");
> -
> -        overlay_file, source
> -    ) source.s_disks in
> +  let overlays = (
> +    if not in_place then (
> +      (* Create a qcow2 v3 overlay to protect the source image(s).  There
> +       * is a specific reason to use the newer qcow2 variant: Because the
> +       * L2 table can store zero clusters efficiently, and because
> +       * discarded blocks are stored as zero clusters, this should allow
us
> +       * to fstrim/blkdiscard and avoid copying significant parts of the
> +       * data over the wire.
> +       *)
> +      message (f_"Creating an overlay to protect the source from
being modified");
> +      List.map (
> +        fun ({ s_qemu_uri = qemu_uri; s_format = format } as source) ->
> +          let overlay_file > +            Filename.temp_file
~temp_dir:overlay_dir "v2vovl" ".qcow2" in
> +          unlink_on_exit overlay_file;
> +
> +          let options > +            "compat=1.1" ^
> +              (match format with None -> ""
> +              | Some fmt -> ",backing_fmt=" ^ fmt) in
> +          let cmd > +            sprintf "qemu-img create -q -f
qcow2 -b %s -o %s %s"
> +              (quote qemu_uri) (quote options) overlay_file in
> +          if verbose () then printf "%s\n%!" cmd;
> +          if Sys.command cmd <> 0 then
> +            error (f_"qemu-img command failed, see earlier
errors");
> +
> +          (* Sanity check created overlay (see below). *)
> +          if not ((new G.guestfs ())#disk_has_backing_file overlay_file)
then
> +            error (f_"internal error: qemu-img did not create overlay
with backing file");
> +
> +          overlay_file, source
> +      ) source.s_disks
> +    ) else []
The overlays array shouldn't be an empty list in the case where we are
doing an in_place conversion.  It should be None (and the other case
should be Some overlays).

However this makes me wonder if there is a better way to structure
this code.  As it stands, when using --in-place, the output object is
set (pretty much randomly) to Output_libvirt.output_libvirt.  So a
bunch of methods on Output_libvirt.output_libvirt are called which
don't make sense.

In virt-sparsify, --in-place is handled essentially by a completely
different main function in a different module, and that might be a
less error-prone way to do things here too.
> +  ) in
>  
>    (* Open the guestfs handle. *)
> -  message (f_"Opening the overlay");
> +  if in_place then
> +    message (f_"Opening the guest disks")
> +  else
> +    message (f_"Opening the overlay");
Change the if statements around so that

  if not in_place then
    (* old code *)
  else
    (* new code *)

I envisage that the not in_place route will remain the usual way to
use virt-v2v.

But as I said above, a separate in_place.ml file may be better.
>    let g = new G.guestfs () in
>    if trace () then g#set_trace true;
>    if verbose () then g#set_verbose true;
>    g#set_network true;
> -  List.iter (
> -    fun (overlay_file, _) ->
> -      g#add_drive_opts overlay_file
> -        ~format:"qcow2" ~cachemode:"unsafe"
~discard:"besteffort"
> -        ~copyonread:true
> -  ) overlays;
> +  if in_place then (
> +    List.iter (
> +      fun ({s_qemu_uri = qemu_uri; s_format = format}) ->
> +        match format with
> +        | None ->
> +            g#add_drive_opts qemu_uri ~cachemode:"unsafe"
~discard:"besteffort"
> +        | Some fmt ->
> +            g#add_drive_opts qemu_uri ~format:fmt
~cachemode:"unsafe"
> +              ~discard:"besteffort"
> +    ) source.s_disks
> +  ) else (
> +    List.iter (
> +      fun (overlay_file, _) ->
> +        g#add_drive_opts overlay_file
> +          ~format:"qcow2" ~cachemode:"unsafe"
~discard:"besteffort"
> +          ~copyonread:true
> +    ) overlays
> +  );
>  
>    g#launch ();
>  
> @@ -294,6 +313,9 @@ let rec main () >    g#shutdown ();
>    g#close ();
>  
> +  if in_place then
> +    exit 0;
> +
>    (* Does the guest require UEFI on the target? *)
>    message (f_"Checking if the guest needs BIOS or UEFI to
boot");
>    let target_firmware > diff --git a/v2v/virt-v2v.pod
b/v2v/virt-v2v.pod
> index ef2dee1..eda1cf7 100644
> --- a/v2v/virt-v2v.pod
> +++ b/v2v/virt-v2v.pod
> @@ -9,6 +9,8 @@ virt-v2v - Convert a guest to use KVM
>   virt-v2v -ic vpx://vcenter.example.com/Datacenter/esxi vmware_guest \
>     -o rhev -os rhev.nfs:/export_domain --network rhevm
>  
> + virt-v2v -ic qemu:///system qemu_guest --in-place
> +
>   virt-v2v -i libvirtxml guest-domain.xml -o local -os /var/tmp
>  
>   virt-v2v -i disk disk.img -o local -os /var/tmp
> @@ -75,6 +77,9 @@ booting the guest directly in qemu (mainly for testing).
>  I<-o rhev> is used to write to a RHEV-M / oVirt target.  I<-o
vdsm>
>  is only used when virt-v2v runs under VDSM control.
>  
> +I<--in-place> instructs virt-v2v to customize the guest OS in the
input
> +virtual machine, instead of creating a new VM in the target hypervisor.
> +
>  =head1 EXAMPLES
>  
>  =head2 Convert from VMware vCenter server to local libvirt
> @@ -518,6 +523,18 @@ C<root>.
>  You will get an error if virt-v2v is unable to mount/write to the
>  Export Storage Domain.
>  
> +=item B<--in-place>
This appears in the wrong place in the list of options (sorted by 'p'
rather than 'i').  I don't know if this was intentional, but I think
it's wrong.
> +Do not create an output virtual machine in the target hypervisor.
> +Instead, adjust the guest OS in the source VM to run in the input
> +hypervisor.
> +
> +This mode is meant for integration with other toolsets, which take the
> +responsibility of converting the VM configuration, providing for
> +rollback in case of errors, transforming the storage, etc.
> +
> +Conflicts with all I<-o *> options.
> +
>  =item B<--password-file> file
>  
>  Instead of asking for password(s) interactively, pass the password
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

On Tue, Jul 28, 2015 at 11:54:59AM +0100, Richard W.M. Jones
wrote:
> In virt-sparsify, --in-place is handled essentially by a completely
> different main function in a different module, and that might be a
> less error-prone way to do things here too.
See:

https://github.com/libguestfs/libguestfs/blob/master/sparsify/copying.ml

 - "traditional" copying virt-sparsify main function

https://github.com/libguestfs/libguestfs/blob/master/sparsify/in_place.ml

 - virt-sparsify --in-place main function

https://github.com/libguestfs/libguestfs/blob/master/sparsify/sparsify.ml#L36-L42

 - selects which main function to call

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-df lists disk usage of guests without needing to install any
software inside the virtual machine.  Supports Linux and Windows.
http://people.redhat.com/~rjones/virt-df/

First off, thanks for the review.  IIUC you are not opposed to the idea
of adding the in-place operation mode in general, are you?  I'll work
through your comments to make the patch fit better with the rest of the
code.

On Tue, Jul 28, 2015 at 11:54:59AM +0100, Richard W.M. Jones
wrote:
> On Mon, Jul 27, 2015 at 07:29:57PM +0300, Roman Kagan wrote:
> > --- a/tests/guests/guests.xml.in
> > +++ b/tests/guests/guests.xml.in
> 
> Let's not add this to tests/guests, since this disk image is only used
> for a single test in the v2v/ subdirectory.
> 
> You can change the new test in the v2v/ subdirectory so it creates (or
> is supplied with) the XML.  There are various tests which work like
> that already - eg: v2v/test-v2v-cdrom.*
OK, makes sense, thanks for the suggestion.

While at this: do you think adding the test for the newly added option
should go in the same commit with the option itself?  It may be easier
to review separately, but YMMV, so please let me know which way you'd
like it better.
> > +  let overlays = (
> > +    if not in_place then (
[...]
> > +    ) else []
> 
> The overlays array shouldn't be an empty list in the case where we are
> doing an in_place conversion.  It should be None (and the other case
> should be Some overlays).
> 
> However this makes me wonder if there is a better way to structure
> this code.  As it stands, when using --in-place, the output object is
> set (pretty much randomly) to Output_libvirt.output_libvirt.  So a
> bunch of methods on Output_libvirt.output_libvirt are called which
> don't make sense.
Right.  Actually when developing the patch I hesitated between better
structure and less changes, and chose the latter as I wasn't sure the
feature would be welcome at all.  That's why I used empty lists here
(and all over the place): as a result I didn't need multiple extra
conditionals, because the code which wasn't supposed to execute in
in_place case didn't as it was passed an empty list to work on.

That said, if we're up to restructuring (which is IMO desirable here
anyway since the main function is now about 400 lines) I'll try to
make it look more natural.
> In virt-sparsify, --in-place is handled essentially by a completely
> different main function in a different module, and that might be a
> less error-prone way to do things here too.
Thanks for the suggestion, I'll have a look.
> Change the if statements around so that
> 
>   if not in_place then
>     (* old code *)
>   else
>     (* new code *)
> 
> I envisage that the not in_place route will remain the usual way to
> use virt-v2v.
OK
> > @@ -518,6 +523,18 @@ C<root>.
> >  You will get an error if virt-v2v is unable to mount/write to the
> >  Export Storage Domain.
> >  
> > +=item B<--in-place>
> 
> This appears in the wrong place in the list of options (sorted by
'p'
> rather than 'i').  I don't know if this was intentional, but I
think
> it's wrong.
I didn't realize the options went in a strict sorting order.  Besides,
at first I thought I'd make it a new -o suboption; then I changed my
mind but didn't bother to adjust the position in the list.

Thanks,
Roman.

This series is a second attempt to add a mode of virt-v2v operation
where it leaves the config and disk image conversion, rollback on
errors, registering with the destination hypervisor, etc. to a
third-party toolset, and performs only tuning of the guest OS to run in
the KVM-based hypervisor.

The first 14 patches are just refactoring and rearrangement of the code,
factoring the implementation details out into separate functions (one
logical step at a time).  This results in main() compacted from a few
hundreds lines to a few dozens containing only coarse steps and making
the scenarios easy to follow.

The last three patches add the new mode, the description of it in the
man page, and a test for it, resp.

Roman Kagan (17):
  v2v: debug gc via at_exit hook
  v2v: factor out opening input VM
  v2v: factor out overlay creation
  v2v: factor out populating targets list
  v2v: factor out size checks
  v2v: factor out actual guest transformation
  v2v: factor out determing the guest firmware
  v2v: move target_bus_assignment ahead of main
  v2v: factor out copying of output data
  v2v: factor out preserving overlays for debugging
  v2v: move main to the end of file
  v2v: drop redundant umount_all() and shutdown()
  v2v: factor out opening guestfs handle
  v2v: factor out populating guestfs with overlays
  v2v: add --in-place mode
  v2v: document --in-place
  v2v: add test for --in-place

---
changes from v1:
 - include refactoring patches before the --in-place ones
 - split --in-place patches into code, doc, and test for easier review
   (bisectability maintained)

 v2v/Makefile.am          |    1 +
 v2v/cmdline.ml           |    7 +-
 v2v/test-v2v-in-place.sh |  101 +++++
 v2v/v2v.ml               | 1056 ++++++++++++++++++++++++----------------------
 v2v/virt-v2v.pod         |   17 +
 5 files changed, 674 insertions(+), 508 deletions(-)
 create mode 100755 v2v/test-v2v-in-place.sh

-- 
2.4.3