[Libguestfs] [PATCH v7 6/6] v2v: Add -o rhv-upload output mode (RHBZ#1557273).

Fri Mar 23 16:19:54 UTC 2018

On Thursday, 22 March 2018 16:24:25 CET Richard W.M. Jones wrote:
> PROBLEMS:
>  - -of qcow2 does not work, with multiple problems
>     * needs to set NBD size to something larger than virtual size
>  - Cannot choose the datacenter.
>  - Not tested against imageio which supports zero/trim/flush.
> 
> This adds a new output mode to virt-v2v.  virt-v2v -o rhv-upload
> streams images directly to an oVirt or RHV >= 4 Data Domain using the
> oVirt SDK v4.  It is more efficient than -o rhv because it does not
> need to go via the Export Storage Domain, and is possible for humans
> to use unlike -o vdsm.
> 
> The implementation uses the Python SDK (‘ovirtsdk4’ module).  An
> nbdkit Python 3 plugin translates NBD calls from qemu into HTTPS
> requests to oVirt via the SDK.
> ---

Regarding the Python scripts: wouldn't it be easier to install them
in $datadir, and run them from that location? This would avoid the
effort of embedding them at build time, and save them in a temporary
location at runtime. IIRC we do something similar already for p2v,
see VIRT_P2V_DATA_DIR.

Also, I'd run pep8 on them (python2-pep8 / python3-pep8 in Fedora),
to make sure their coding style follows PEP8.

> diff --git a/v2v/cmdline.ml b/v2v/cmdline.ml
> index dc675fb42..bdd3127f8 100644
> --- a/v2v/cmdline.ml
> +++ b/v2v/cmdline.ml
> @@ -137,6 +137,8 @@ let parse_cmdline () =
>      | "disk" | "local" -> output_mode := `Local
>      | "null" -> output_mode := `Null
>      | "ovirt" | "rhv" | "rhev" -> output_mode := `RHV
> +    | "ovirt-upload" | "ovirt_upload" | "rhv-upload" | "rhv_upload" ->
> +       output_mode := `RHV_Upload

I'd not use the variants with underscores, simply to not have too many
options possible.

> diff --git a/v2v/embed.sh b/v2v/embed.sh
> new file mode 100755
> index 000000000..0a65cd428
> --- /dev/null
> +++ b/v2v/embed.sh
> @@ -0,0 +1,45 @@
> +#!/bin/bash -
> +# Embed code or other content into an OCaml file.
> +# Copyright (C) 2018 Red Hat Inc.
> +#
> +# 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.
> +
> +# Embed code or other content into an OCaml file.
> +#
> +# It is embedded into a string.  As OCaml string literals have virtually
> +# no restrictions on length or content we only have to escape double
> +# quotes for backslash characters.

This scripts needs set -eu, so it fails in case of any failure of its
commands.

> +  (* JSON parameters which are invariant between disks. *)
> +  let json_params = [
> +    "output_conn", JSON.String output_conn;
> +    "output_password", JSON.String output_password;
> +    "output_storage", JSON.String output_storage;
> +    "output_sparse", JSON.Bool (match output_alloc with
> +                                | Sparse -> true
> +                                | Preallocated -> false);
> +    "rhv_cafile", JSON.String rhv_options.rhv_cafile;
> +    "rhv_cluster",
> +      JSON.String (Option.default "Default" rhv_options.rhv_cluster);
> +    "rhv_direct", JSON.Bool rhv_options.rhv_direct;
> +
> +    (* The 'Insecure' flag seems to be a number with various possible
> +     * meanings, however we just set it to True/False.
> +     *
> +     * https://github.com/oVirt/ovirt-engine-sdk/blob/19aa7070b80e60a4cfd910448287aecf9083acbe/sdk/lib/ovirtsdk4/__init__.py#L395
> +     *)
> +    "insecure", JSON.Bool (not rhv_options.rhv_verifypeer);

I'd make the comment match the name of the key, so it is easier to find
if needed.

> +object
> +  inherit output
> +
> +  method precheck () =

Considering python3 is directly run by this output mode, it'd be fair
to check for an installed python3 here, using Std_utils.which.

> +    (* Create an nbdkit instance for each disk and set the
> +     * target URI to point to the NBD socket.
> +     *)
> +    List.map (
> +      fun t ->
> +        let id = t.target_overlay.ov_source.s_disk_id in
> +        let disk_name = sprintf "%s-%03d" output_name id in
> +        let json_params =
> +          ("disk_name", JSON.String disk_name) :: json_params in
> +
> +        let disk_format =
> +          match t.target_format with
> +          | ("raw" | "qcow2") as fmt -> fmt
> +          | _ ->
> +             error (f_"rhv-upload: -of %s: Only output format ‘raw’ or ‘qcow2’ is supported.  If the input is in a different format then force one of these output formats by adding either ‘-of raw’ or ‘-of qcow2’ on the command line.")
> +                   t.target_format in
> +        let json_params =
> +          ("disk_format", JSON.String disk_format) :: json_params in
> +
> +        let disk_size = t.target_overlay.ov_virtual_size in
> +        let json_params =
> +          ("disk_size", JSON.Int64 disk_size) :: json_params in
> +
> +        (* Ask the plugin to write the disk ID to a special file. *)
> +        let diskid_file = diskid_file_of_id id in
> +        let json_params =
> +          ("diskid_file", JSON.String diskid_file) :: json_params in

Maybe grouping these parameters in an array, e.g.:

  let json_params = [
    "disk_name", JSON.String disk_name;
    "disk_format", JSON.String disk_format;
    "disk_size", JSON.Int64 t.target_overlay.ov_virtual_size;
    "diskid_file", JSON.String (diskid_file_of_id id);
  ] @ json_params in

> diff --git a/v2v/virt-v2v.pod b/v2v/virt-v2v.pod
> index 4e3daedf0..caf9c983e 100644
> --- a/v2v/virt-v2v.pod
> +++ b/v2v/virt-v2v.pod
> - virt-v2v -o vdsm -oo "?"
> + virt-v2v -o rhv-upload -oo "?"
> +
> +=item B<-oo rhv-cafile=>F<ca.pem>
> +
> +For I<-o rhv-upload> (L<OUTPUT TO RHV>) only, the F<ca.pem> file

It must be L</OUTPUT TO RHV>, otherwise it is considered as man page.
(There are various occurrences of this in this patch.)

-- 
Pino Toscano
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: This is a digitally signed message part.
URL: <http://listman.redhat.com/archives/libguestfs/attachments/20180323/95ee06ad/attachment.sig>