Libguestfs - Aug 2015 - Re: [PATCH v2 15/17] v2v: add --in-place mode

On Tue, Aug 11, 2015 at 08:00:34PM +0300, Roman Kagan
wrote:
> +  let overlays > +    if not in_place then create_overlays
source.s_disks
> +    else [] in
> +  let targets > +    if not in_place then init_targets overlays source
output output_format
> +    else [] in
This doesn't solve the problem I raised before which is that overlays
and targets should never be empty lists.

I think really what should be happening here is something like this:

(1) Create extra modules:

  common.ml - for common functions shared; patches 1-14 will end up
   moving a lot of functions into this file

  copying.ml - for copying conversion (ie. what virt-v2v does now)

  in_place.ml - for in-place conversion

(2) v2v.ml calls out to either Copying or In_place -- see how
  virt-sparsify works.

in_place.ml would not need to have overlays & targets at all,
hence the issue above goes away.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-top is 'top' for virtual machines.  Tiny program with many
powerful monitoring features, net stats, disk stats, logging, etc.
http://people.redhat.com/~rjones/virt-top

On Thu, Aug 27, 2015 at 04:08:43PM +0100, Richard W.M. Jones
wrote:
> On Tue, Aug 11, 2015 at 08:00:34PM +0300, Roman Kagan wrote:
> > +  let overlays > > +    if not in_place then create_overlays
source.s_disks
> > +    else [] in
> > +  let targets > > +    if not in_place then init_targets
overlays source output output_format
> > +    else [] in
> 
> This doesn't solve the problem I raised before which is that overlays
> and targets should never be empty lists.
Why shouldn't they?

I thought the problem was that it was hard to keep track of whether the
empty lists made sense deep inside the main()'s guts; now that main()
fits in a screen it's easy to see that there are just two scenarios
sharing some steps but not others, and overlays and targets are empty
just when those steps aren't taken.

Moreover the steps that operated on overlays or targets are made
explicitly conditional on !in_place, even though they are tolerant of
empty lists.

So what's the problem with empty lists, please?
> I think really what should be happening here is something like this:
> 
> (1) Create extra modules:
> 
>   common.ml - for common functions shared; patches 1-14 will end up
>    moving a lot of functions into this file
> 
>   copying.ml - for copying conversion (ie. what virt-v2v does now)
> 
>   in_place.ml - for in-place conversion
> 
> (2) v2v.ml calls out to either Copying or In_place -- see how
>   virt-sparsify works.
I actually followed the advice you gave in the previous review and did
look into virt-sparsify.  I must admit I didn't appreciate the amount of
code duplication between the two usage scenarios.  That was exactly what
I wanted to avoid in my submission.

The posted code covers two usage scenarios like this:

  - common steps (command line, initialization)
  - conditional steps (creation of overlays, guestfs handle with either
    overlays or original disks)
  - common steps (inspection, space estimate, conversion)
  - conditional steps (creation of destination VM)

That's basically all to it.  How can this conveniently be split into
independent scenarios without code duplication and the risk of
forgetting to update one of the two when the common steps are being
modified?

Roman.

On Thu, Aug 27, 2015 at 09:39:30PM +0300, Roman Kagan
wrote:
> On Thu, Aug 27, 2015 at 04:08:43PM +0100, Richard W.M. Jones wrote:
> > On Tue, Aug 11, 2015 at 08:00:34PM +0300, Roman Kagan wrote:
> > > +  let overlays > > > +    if not in_place then
create_overlays source.s_disks
> > > +    else [] in
> > > +  let targets > > > +    if not in_place then
init_targets overlays source output output_format
> > > +    else [] in
> > 
> > This doesn't solve the problem I raised before which is that
overlays
> > and targets should never be empty lists.
> 
> Why shouldn't they?
Because the point of using the language is to use its type safety to
ensure there are no errors, now or in the future.  I reproduced your
new main() function after the patch series at the end of this email so
we have something concrete to look at.

At the moment, it's (sort of) obvious that

  in_place => overlays = targets = []

but we're not using the compiler to enforce that, so it could break in
future.

It can be made type-safe without a huge amount of work.  Something
like this.  First define a new type:

type conversion_mode   | Copying of overlay list * target list
  | In_place

Then change the code to look like below (I didn't change all of it,
but it should be fairly obvious from the examples here):

  let conversion_mode     if not in_place then
      Copying (
       let overlays = create_overlays source.s_disks in
       let targets = init_targets overlays source output output_format in
       overlays, targets)
    else In_place in

...

  (match conversion_mode with
    Copying (overlays, _) -> populate_overlays g overlays
    In_place ->  populate_disks g source.s_disks
  );

...

  match conversion_mode with
  | In_place ->
      message (f_"Finishing off");
      exit 0
  | Copying (overlays, targets) ->
      (* rest of main function follows here ... *)


> > (2) v2v.ml calls out to either Copying or In_place -- see how
> >   virt-sparsify works.
> 
> I actually followed the advice you gave in the previous review and did
> look into virt-sparsify.  I must admit I didn't appreciate the amount
of
> code duplication between the two usage scenarios.  That was exactly what
> I wanted to avoid in my submission.
> 
> The posted code covers two usage scenarios like this:
> 
>   - common steps (command line, initialization)
>   - conditional steps (creation of overlays, guestfs handle with either
>     overlays or original disks)
>   - common steps (inspection, space estimate, conversion)
>   - conditional steps (creation of destination VM)
> 
> That's basically all to it.  How can this conveniently be split into
> independent scenarios without code duplication and the risk of
> forgetting to update one of the two when the common steps are being
> modified?
OK so you've convinced me that a Common code module isn't
necessary and would lead to a lot of duplication.

However - I still don't like the bottom-up reworking of the code, and
changing that means having fewer, simpler patches in this series.  And
I think the in-place/copying stuff can be made type-safe as above.

Rich.


----------------------------------------------------------------------

let main ()   (* Handle the command line. *)
  let input, output,
    debug_gc, debug_overlays, do_copy, in_place, network_map, no_trim,
    output_alloc, output_format, output_name, print_source, root_choice    
Cmdline.parse_cmdline () in

  (* Print the version, easier than asking users to tell us. *)
  if verbose () then
    printf "%s: %s %s (%s)\n%!"
      prog Config.package_name Config.package_version Config.host_cpu;

  if debug_gc then
    at_exit (fun () -> Gc.compact());

  let source = open_source input print_source in
  let source = amend_source source output_name network_map in
  let overlays     if not in_place then create_overlays source.s_disks
    else [] in
  let targets     if not in_place then init_targets overlays source output
output_format
    else [] in

  let guestfs_kind     if not in_place then "overlay" else
"source VM" in
  message (f_"Opening the %s") guestfs_kind;
  let g = open_guestfs () in

  if not in_place then populate_overlays g overlays
  else populate_disks g source.s_disks;

  g#launch ();

  (* Inspection - this also mounts up the filesystems. *)
  message (f_"Inspecting the %s") guestfs_kind;
  let inspect = inspect_source g root_choice in

  let mpstats = get_mpstats g in
  check_free_space mpstats;
  if not in_place then
    check_target_free_space mpstats source targets output;

  let keep_serial_console     if not in_place then output#keep_serial_console
    else true in
  let guestcaps = do_convert g inspect source keep_serial_console in

  if no_trim <> ["*"] && (do_copy || debug_overlays)
then (
    (* Doing fstrim on all the filesystems reduces the transfer size
     * because unused blocks are marked in the overlay and thus do
     * not have to be copied.
     *)
    message (f_"Mapping filesystem data to avoid copying unused and blank
areas");
    do_fstrim g no_trim inspect;
  );

  message (f_"Closing the %s") guestfs_kind;
  g#close ();

  if in_place then (
    message (f_"Finishing off");
    exit 0
  );

  let target_firmware = get_target_firmware inspect guestcaps source output in
  let target_buses = target_bus_assignment source targets guestcaps in
  let targets     if not do_copy then targets
    else copy_targets targets input output output_alloc in

  (* Create output metadata. *)
  message (f_"Creating output metadata");
  output#create_metadata source targets target_buses guestcaps inspect
                         target_firmware;

  if debug_overlays then preserve_overlays overlays source.s_name;

  message (f_"Finishing off");
  delete_target_on_exit := false  (* Don't delete target on exit. *)
----------------------------------------------------------------------


-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-top is 'top' for virtual machines.  Tiny program with many
powerful monitoring features, net stats, disk stats, logging, etc.
http://people.redhat.com/~rjones/virt-top