Libguestfs - Feb 2016 - Re: [PATCH v2 4/4] v2v: in-place: request caps based on source config

On Sat, Feb 20, 2016 at 11:26:10AM +0300, Roman Kagan
wrote:
> In in-place mode, the decisions on which interfaces to use are made and
> the source configuration is created by the outside entity.  So in that
> case v2v needs to look it up in the source configuraion, and try to
> follow.
> 
> For that, the source configuration is used to populate requested caps
> object which is then passed to the convert routine.
> 
> Signed-off-by: Roman Kagan <rkagan@virtuozzo.com>
> ---
> 
> Notes:
>     v2:
>      - accept catch-all variants of source net and video as no preference
> 
>  v2v/types.mli |  6 +++---
>  v2v/v2v.ml    | 55 ++++++++++++++++++++++++++++++++++++++++++++++++++-----
>  2 files changed, 53 insertions(+), 8 deletions(-)
> 
> diff --git a/v2v/types.mli b/v2v/types.mli
> index 0e40668..fbd45cf 100644
> --- a/v2v/types.mli
> +++ b/v2v/types.mli
> @@ -66,7 +66,7 @@ and source_disk = {
>  (** A source disk. *)
>  
>  and s_controller = Source_IDE | Source_SCSI | Source_virtio_blk
> -(** Source disk controller.
> +(** Source disk controller (in ascending order of preference).
>  
>      For the purposes of this field, we can treat virtio-scsi as
>      [SCSI].  However we don't support conversions from virtio in any
> @@ -88,7 +88,7 @@ and source_nic = {
>    s_vnet_orig : string;                 (** Original network (if we map
it). *)
>    s_vnet_type : vnet_type;              (** Source network type. *)
>  }
> -(** Network adapter models. *)
> +(** Network adapter models (in ascending order of preference). *)
>  and s_nic_model = Source_other_nic of string |
>                    Source_rtl8139 | Source_e1000 | Source_virtio_net
>  (** Network interfaces. *)
> @@ -108,7 +108,7 @@ and s_display_listen >    | LAddress of string      
(** Listen address. *)
>    | LNetwork of string             (** Listen network. *)
>  
> -(** Video adapter model. *)
> +(** Video adapter model (in ascending order of preference). *)
>  and source_video = Source_other_video of string |
>                     Source_Cirrus | Source_QXL
>  
> diff --git a/v2v/v2v.ml b/v2v/v2v.ml
> index c828e48..608150f 100644
> --- a/v2v/v2v.ml
> +++ b/v2v/v2v.ml
> @@ -82,11 +82,17 @@ let rec main () >    );
>  
>    let keep_serial_console = output#keep_serial_console in
> -  let rcaps = {
> -                rcaps_block_bus = None;
> -                rcaps_net_bus = None;
> -                rcaps_video = None;
> -              } in
> +  let rcaps > +    match conversion_mode with
> +    | Copying (_, _) ->
You can just write this:

   | Copying _ ->
> +        {
> +          rcaps_block_bus = None;
> +          rcaps_net_bus = None;
> +          rcaps_video = None;
> +        }
> +    | In_place ->
> +        rcaps_from_source source
> +  in
>    let guestcaps = do_convert g inspect source keep_serial_console rcaps in
>  
>    g#umount_all ();
> @@ -974,4 +980,43 @@ and preserve_overlays overlays src_name >       
printf (f_"Overlay saved as %s [--debug-overlays]\n") saved_filename
>    ) overlays
>  
> +and rcaps_from_source source > +  (* Request guest caps based on source
configuration. *)
> +
> +  (* rely on s_controller enum being in ascending preference order, and
None
> +   * being smaller than Some anything *)
> +  let best_block_type > +    List.fold_left max None
> +      (List.map (fun sd -> sd.s_controller) source.s_disks) in
> +  let block_type > +    match best_block_type with
> +    | Some Source_virtio_blk -> Some Virtio_blk
> +    | Some Source_SCSI -> None
> +    | Some Source_IDE -> Some IDE
> +    | None -> None in
> +
> +  (* rely on s_nic_model enum being in ascending preference order, and
None
> +   * being smaller than Some anything *)
> +  let best_net_type > +    List.fold_left max None
> +      (List.map (fun nic -> nic.s_nic_model) source.s_nics) in
> +  let net_type > +    match best_net_type with
> +    | Some Source_virtio_net -> Some Virtio_net
> +    | Some Source_e1000 -> Some E1000
> +    | Some Source_rtl8139 -> Some RTL8139
> +    | Some Source_other_nic _ | None -> None in
> +
> +  let video > +    match source.s_video with
> +    | Some Source_QXL -> Some QXL
> +    | Some Source_Cirrus -> Some Cirrus
> +    | Some Source_other_video _ | None -> None in
> +
> +  {
> +    rcaps_block_bus = block_type;
> +    rcaps_net_bus = net_type;
> +    rcaps_video = video;
> +  }
> +
>  let () = run_main_and_handle_errors main
It's a bit surprising, because I thought that you'd want to pass in
the requested preferences on the command line?

About the specific issue of ordering, although what you've written
works, you could find a few surprises.  eg:

  $ rlwrap ocaml
          OCaml version 4.02.3
  
  # type t = A | B | C ;;
  type t = A | B | C
  # A < C ;;
  - : bool = true
  # A < B ;;
  - : bool = true
  # B < C ;;
  - : bool = true

but ...

  # type t = A | B of int | C ;;
  type t = A | B of int | C
  # A < C ;;
  - : bool = true
  # A < B 3 ;;
  - : bool = true
  # B 3 < C ;;
  - : bool = false

This happens because adding a parameter to an enum changes the
internal representation.

So the code as written could stop working with innocuous changes to
the declared types, or even in a future version of OCaml, because it
depends on internals of the OCaml representation.

IMHO it's safer to make the ordering you want explicit.  So:

  let precedence_of_s_controller_option = function
    | None -> 0 (* worst *)
    | Some Source_IDE -> 1
    | Some Source_SCSI -> 2
    | Some Source_virtio_blk -> 3 (* best *)

and then you can have a function that finds the best "something" in a
list of "somethings", using a polymorphic precedence function to
compare the best:

  let rec find_best precedence = function
    | [] -> None, 0
    | x :: xs ->
      let precedence_of_x = precedence x
      and best_in_xs, precedence_of_xs = find_best precedence xs in
      if precedence_of_x > precedence_of_xs then
        x, precedence_of_x
      else
        best_in_xs, precedence_of_xs

This function has type:

  val find_best : ('a option -> int) -> 'a option list ->
'a option * int

(Note it returns a pair: the best choice and also the precedence
integer of that choice).

The code to find the best block_type or net_type falls out fairly easily:

  let block_type     let sctrls = List.map (fun sd -> sd.s_controller)
source.s_disks in
    let best, _ = find_best precedence_of_s_controller_option sctrls in
    match best with
    | Some Source_virtio_blk -> Some Virtio_blk
    | Some Source_SCSI -> None
    | Some Source_IDE -> Some IDE
    | None -> None in

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/

On Mon, Feb 22, 2016 at 03:04:57PM +0000, Richard W.M. Jones
wrote:
> On Sat, Feb 20, 2016 at 11:26:10AM +0300, Roman Kagan wrote:
> > In in-place mode, the decisions on which interfaces to use are made
and
> > the source configuration is created by the outside entity.  So in that
> > case v2v needs to look it up in the source configuraion, and try to
> > follow.
> > 
> > For that, the source configuration is used to populate requested caps
> > object which is then passed to the convert routine.
[...]
> > +and rcaps_from_source source > > +  (* Request guest caps based
on source configuration. *)
> > +
> > +  (* rely on s_controller enum being in ascending preference order,
and None
> > +   * being smaller than Some anything *)
> > +  let best_block_type > > +    List.fold_left max None
> > +      (List.map (fun sd -> sd.s_controller) source.s_disks) in
> > +  let block_type > > +    match best_block_type with
> > +    | Some Source_virtio_blk -> Some Virtio_blk
> > +    | Some Source_SCSI -> None
> > +    | Some Source_IDE -> Some IDE
> > +    | None -> None in
> > +
> > +  (* rely on s_nic_model enum being in ascending preference order,
and None
> > +   * being smaller than Some anything *)
> > +  let best_net_type > > +    List.fold_left max None
> > +      (List.map (fun nic -> nic.s_nic_model) source.s_nics) in
> > +  let net_type > > +    match best_net_type with
> > +    | Some Source_virtio_net -> Some Virtio_net
> > +    | Some Source_e1000 -> Some E1000
> > +    | Some Source_rtl8139 -> Some RTL8139
> > +    | Some Source_other_nic _ | None -> None in
> > +
> > +  let video > > +    match source.s_video with
> > +    | Some Source_QXL -> Some QXL
> > +    | Some Source_Cirrus -> Some Cirrus
> > +    | Some Source_other_video _ | None -> None in
> > +
> > +  {
> > +    rcaps_block_bus = block_type;
> > +    rcaps_net_bus = net_type;
> > +    rcaps_video = video;
> > +  }
> > +
> >  let () = run_main_and_handle_errors main
> 
> It's a bit surprising, because I thought that you'd want to pass in
> the requested preferences on the command line?
Passing that on the command line makes sense for the copying mode,
because in that mode there's no other place where to affect the choices.

In the in-place mode virt-v2v is given a VM with the configuration
already converted and the choices made (with or without user
interaction); v2v is only expected to tune the guest to work in that
configuration.  So in that case rcaps are based on the source (=target) VM
properties.
> About the specific issue of ordering, although what you've written
> works, you could find a few surprises.  eg:
I suspected that, but failed to come up with a cleaner approach like the
one you suggested.  Thanks, I'll rework the patch and resubmit.

Roman.

On Wed, Feb 24, 2016 at 05:33:44PM +0300, Roman Kagan
wrote:
> Passing that on the command line makes sense for the copying mode,
> because in that mode there's no other place where to affect the
choices.
> 
> In the in-place mode virt-v2v is given a VM with the configuration
> already converted and the choices made (with or without user
> interaction); v2v is only expected to tune the guest to work in that
> configuration.  So in that case rcaps are based on the source (=>
target) VM properties.
I think this makes 'virt-v2v --inplace' a bit less useful as a general
thing you can do with the tool.  Can we switch on this behaviour only
if there is a command line flag?  That would ensure that 'virt-v2v
--inplace' is a generally useful feature for people who want virt-v2v
to install the best drivers, while your use case is still possible (by
passing the flag).

Alternately (and a bit more work): expose the requested guestcaps
completely through the command line, eg:

  --request-block=[virtio|ide|best]
  --request-net=[virtio|e1000|rtl8139|best]
  --request-video=[qxl|cirrus|best]

where 'best' == None (I think?)

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
libguestfs lets you edit virtual machines.  Supports shell scripting,
bindings from many languages.  http://libguestfs.org