[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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



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


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]