Libguestfs - Feb 2018 - [PATCH 0/5] v2v: Add -o rhv-upload output mode.

The first four patches are straightforward.

The final patch adds the -o rhv-upload output mode.  It is
still spooling into a temporary file because I've had some
trouble getting streaming conversions working.  There are
other problems as outlined in the commit message, so this
patch is not ready for upstream but is good for discussion.

Also I hit this, which I'm assuming for now will be fixed
by Tomas's work:

  ovirtsdk4.Error: Fault reason is "Operation Failed". Fault detail is
"failed to parse a given ovf configuration ovf error: [empty name]: cannot
read '//*/disksection' with value: null". HTTP response code is
400.

Rich.

Convert a document to an in-memory string.
---
 v2v/DOM.ml  | 61 ++++++++++++++++++++++++++++++++++++++-----------------------
 v2v/DOM.mli |  3 +++
 2 files changed, 41 insertions(+), 23 deletions(-)

diff --git a/v2v/DOM.ml b/v2v/DOM.ml
index 7f66e0920..2b6a21fd1 100644
--- a/v2v/DOM.ml
+++ b/v2v/DOM.ml
@@ -46,43 +46,48 @@ let e name attrs children   * we will be writing, ie.
libvirt XML and OVF metadata, where
  * whitespace is generally not significant, but readability is useful.
  *)
-let rec node_to_chan ?(indent = 0) chan = function
-  | PCData str -> output_string chan (xml_quote_pcdata str)
+let rec node_to_buf ?(indent = 0) buf = function
+  | PCData str ->
+     Buffer.add_string buf (xml_quote_pcdata str)
   | Comment str ->
-    output_spaces chan indent;
-    fprintf chan "<!-- %s -->" (xml_quote_pcdata str)
-  | Element e -> element_to_chan ~indent chan e
-and element_to_chan ?(indent = 0) chan
+     buffer_add_spaces buf indent;
+     bprintf buf "<!-- %s -->" (xml_quote_pcdata str)
+  | Element e ->
+     element_to_buf ~indent buf e
+and element_to_buf ?(indent = 0) buf
     { e_name = name; e_attrs = attrs; e_children = children } -  output_spaces
chan indent;
-  fprintf chan "<%s" name;
-  List.iter (fun (n, v) -> fprintf chan " %s='%s'" n
(xml_quote_attr v)) attrs;
+  buffer_add_spaces buf indent;
+  bprintf buf "<%s" name;
+  List.iter (fun (n, v) -> bprintf buf " %s='%s'" n
(xml_quote_attr v)) attrs;
   if children <> [] then (
-    output_string chan ">";
+    Buffer.add_string buf ">";
     let last_child_was_element = ref false in
     List.iter (
       function
       | Element _ as child ->
         last_child_was_element := true;
-        output_char chan '\n';
-        node_to_chan ~indent:(indent+2) chan child;
+        Buffer.add_char buf '\n';
+        node_to_buf ~indent:(indent+2) buf child;
       | PCData _ as child ->
         last_child_was_element := false;
-        node_to_chan ~indent:(indent+2) chan child;
+        node_to_buf ~indent:(indent+2) buf child;
       | Comment _ as child ->
         last_child_was_element := true;
-        output_char chan '\n';
-        node_to_chan ~indent:(indent+2) chan child;
+        Buffer.add_char buf '\n';
+        node_to_buf ~indent:(indent+2) buf child;
     ) children;
     if !last_child_was_element then (
-      output_char chan '\n';
-      output_spaces chan indent
+      Buffer.add_char buf '\n';
+      buffer_add_spaces buf indent
     );
-    fprintf chan "</%s>" name
+    bprintf buf "</%s>" name
   ) else (
-    output_string chan "/>"
+    Buffer.add_string buf "/>"
   )
 
+and buffer_add_spaces buf n +  for i = 0 to n-1 do Buffer.add_char buf '
' done
+
 (* Quote XML <element attr='...'> content.  Note you must use
single
  * quotes around the attribute.
  *)
@@ -99,10 +104,20 @@ and xml_quote_pcdata str    let str = String.replace str
">" "&gt;" in
   str
 
-let doc_to_chan chan (Doc doc) -  fprintf chan "<?xml
version='1.0' encoding='utf-8'?>\n";
-  element_to_chan chan doc;
-  fprintf chan "\n"
+let doc_to_buf buf (Doc doc) +  bprintf buf "<?xml
version='1.0' encoding='utf-8'?>\n";
+  element_to_buf buf doc;
+  bprintf buf "\n"
+
+let doc_to_string doc +  let buf = Buffer.create 4096 in
+  doc_to_buf buf doc;
+  Buffer.contents buf
+
+let doc_to_chan chan doc +  let buf = Buffer.create 4096 in
+  doc_to_buf buf doc;
+  Buffer.output_buffer chan buf
 
 let path_to_nodes (Doc doc) path    match path with
diff --git a/v2v/DOM.mli b/v2v/DOM.mli
index 1aa89b7e7..99223c389 100644
--- a/v2v/DOM.mli
+++ b/v2v/DOM.mli
@@ -60,6 +60,9 @@ v}
 v}
 *)
 
+val doc_to_string : doc -> string
+(** Convert a document to a string representation. *)
+
 val doc_to_chan : out_channel -> doc -> unit
 (** Write the XML document to an output channel. *)
 
-- 
2.13.2

Currently unused, in a future commit this will allow you to pass in a
password to be used when connecting to the target hypervisor.
---
 v2v/cmdline.ml   | 18 ++++++++++++++++++
 v2v/virt-v2v.pod |  7 +++++++
 2 files changed, 25 insertions(+)

diff --git a/v2v/cmdline.ml b/v2v/cmdline.ml
index efad080cc..059804431 100644
--- a/v2v/cmdline.ml
+++ b/v2v/cmdline.ml
@@ -62,6 +62,7 @@ let parse_cmdline ()    let output_conn = ref None in
   let output_format = ref None in
   let output_name = ref None in
+  let output_password = ref None in
   let output_storage = ref None in
   let password_file = ref None in
   let vddk_config = ref None in
@@ -214,6 +215,8 @@ let parse_cmdline ()                                     
s_"Set output format";
     [ M"on" ],       Getopt.String ("name",
set_string_option_once "-on" output_name),
                                     s_"Rename guest when converting";
+    [ M"op" ],       Getopt.String ("filename",
set_string_option_once "-op" output_password),
+                                    s_"Use password from file to connect
to output hypervisor";
     [ M"os" ],       Getopt.String ("storage",
set_string_option_once "-os" output_storage),
                                     s_"Set output storage location";
     [ L"password-file" ], Getopt.String ("file",
set_string_option_once "--password-file" password_file),
@@ -307,6 +310,7 @@ read the man page virt-v2v(1).
   let output_format = !output_format in
   let output_mode = !output_mode in
   let output_name = !output_name in
+  let output_password = !output_password in
   let output_storage = !output_storage in
   let password_file = !password_file in
   let print_source = !print_source in
@@ -452,6 +456,8 @@ read the man page virt-v2v(1).
     | `Glance ->
       if output_conn <> None then
         error_option_cannot_be_used_in_output_mode "glance"
"-oc";
+      if output_password <> None then
+        error_option_cannot_be_used_in_output_mode "glance"
"-op";
       if output_storage <> None then
         error_option_cannot_be_used_in_output_mode "glance"
"-os";
       if qemu_boot then
@@ -463,6 +469,8 @@ read the man page virt-v2v(1).
 
     | `Not_set
     | `Libvirt ->
+      if output_password <> None then
+        error_option_cannot_be_used_in_output_mode "libvirt"
"-op";
       let output_storage = Option.default "default" output_storage in
       if qemu_boot then
         error_option_cannot_be_used_in_output_mode "libvirt"
"--qemu-boot";
@@ -472,6 +480,8 @@ read the man page virt-v2v(1).
       output_format, output_alloc
 
     | `Local ->
+      if output_password <> None then
+        error_option_cannot_be_used_in_output_mode "local"
"-op";
       let os          match output_storage with
         | None ->
@@ -491,6 +501,8 @@ read the man page virt-v2v(1).
         error_option_cannot_be_used_in_output_mode "null"
"-oc";
       if output_format <> None then
         error_option_cannot_be_used_in_output_mode "null"
"-of";
+      if output_password <> None then
+        error_option_cannot_be_used_in_output_mode "null"
"-op";
       if output_storage <> None then
         error_option_cannot_be_used_in_output_mode "null"
"-os";
       if qemu_boot then
@@ -500,6 +512,8 @@ read the man page virt-v2v(1).
       Some "raw", Sparse
 
     | `QEmu ->
+      if output_password <> None then
+        error_option_cannot_be_used_in_output_mode "qemu"
"-op";
       let os          match output_storage with
         | None ->
@@ -511,6 +525,8 @@ read the man page virt-v2v(1).
       output_format, output_alloc
 
     | `RHV ->
+      if output_password <> None then
+        error_option_cannot_be_used_in_output_mode "rhv"
"-op";
       let os          match output_storage with
         | None ->
@@ -522,6 +538,8 @@ read the man page virt-v2v(1).
       output_format, output_alloc
 
     | `VDSM ->
+      if output_password <> None then
+        error_option_cannot_be_used_in_output_mode "vdsm"
"-op";
       let os          match output_storage with
         | None ->
diff --git a/v2v/virt-v2v.pod b/v2v/virt-v2v.pod
index 4179245d6..4e75995d0 100644
--- a/v2v/virt-v2v.pod
+++ b/v2v/virt-v2v.pod
@@ -569,6 +569,13 @@ If not specified, then the input format is used.
 Rename the guest when converting it.  If this option is not used then
 the output name is the same as the input name.
 
+=item B<-op> file
+
+Supply a file containing a password to be used when connecting to the
+target hypervisor.  Note the file should contain the whole password,
+B<without any trailing newline>, and for security the file should have
+mode C<0600> so that others cannot read it.
+
 =item B<-os> storage
 
 The location of the storage for the converted guest.
-- 
2.13.2

No change in the code or the command line, this is just documenting
that -oc is not specifically a libvirt URI.
---
 v2v/cmdline.ml   |  2 +-
 v2v/virt-v2v.pod | 10 +++++-----
 2 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/v2v/cmdline.ml b/v2v/cmdline.ml
index 059804431..fceda1f82 100644
--- a/v2v/cmdline.ml
+++ b/v2v/cmdline.ml
@@ -210,7 +210,7 @@ let parse_cmdline ()      [ M"oa" ],      
Getopt.String ("sparse|preallocated", set_output_alloc),
                                     s_"Set output allocation mode";
     [ M"oc" ],       Getopt.String ("uri",
set_string_option_once "-oc" output_conn),
-                                    s_"Libvirt URI";
+                                    s_"Output hypervisor connection";
     [ M"of" ],       Getopt.String ("raw|qcow2",
set_string_option_once "-of" output_format),
                                     s_"Set output format";
     [ M"on" ],       Getopt.String ("name",
set_string_option_once "-on" output_name),
diff --git a/v2v/virt-v2v.pod b/v2v/virt-v2v.pod
index 4e75995d0..31d88ef4b 100644
--- a/v2v/virt-v2v.pod
+++ b/v2v/virt-v2v.pod
@@ -550,13 +550,13 @@ This mode is only used when virt-v2v runs under VDSM
control.
 
 Set the output file allocation mode.  The default is C<sparse>.
 
-=item B<-oc> libvirtURI
+=item B<-oc> URI
 
-Specify a libvirt connection to use when writing the converted guest.
-This is only used when S<I<-o libvirt>>.  See L</OUTPUT TO
LIBVIRT> below.
+Specify a connection URI to use when writing the converted guest.
 
-Only local libvirt connections can be used.  Remote libvirt
-connections will not work.
+For S<I<-o libvirt>> this is the libvirt URI.  Only local libvirt
+connections can be used.  Remote libvirt connections will not work.
+See L</OUTPUT TO LIBVIRT> below for further information.
 
 =item B<-of> format
 
-- 
2.13.2

---
 v2v/utils.ml  | 16 ++++++++++++++++
 v2v/utils.mli | 11 +++++++++++
 2 files changed, 27 insertions(+)

diff --git a/v2v/utils.ml b/v2v/utils.ml
index 1ceba94cd..f8dbf233e 100644
--- a/v2v/utils.ml
+++ b/v2v/utils.ml
@@ -196,3 +196,19 @@ let find_file_in_tar tar filename      )
   in
   loop lines
+
+let run_python ?(python = "python") code +  (* In debug mode output
the python code without quoting into
+   * the log, and don't echo the actual command because the
+   * multiple levels of quoting is very confusing.
+   *)
+  debug "running python code using ‘%s’:\n%s" python code;
+  let cmd = sprintf "%s -c %s" (quote python) (quote code) in
+  external_command ~echo_cmd:false cmd
+
+let py_quote str +  let str = String.replace str "\\"
"\\\\" in
+  let str = String.replace str "'" "\\'" in
+  "'''" ^ str ^ "'''"
+
+let py_bool = function true -> "True" | false ->
"False"
diff --git a/v2v/utils.mli b/v2v/utils.mli
index 34402e28f..3a8dfd437 100644
--- a/v2v/utils.mli
+++ b/v2v/utils.mli
@@ -61,3 +61,14 @@ val find_file_in_tar : string -> string -> int64 *
int64
 
     Function raises [Not_found] if there is no such file inside [tar] and
     [Failure] if there is any error parsing the tar output. *)
+
+val run_python : ?python:string -> string -> string list
+(** Run a snippet of Python code, and collect the output.
+
+    On failure this raises an error. *)
+
+val py_quote : string -> string
+(** Return a fully quoted Python string literal. *)
+
+val py_bool : bool -> string
+(** Return "True" or "False". *)
-- 
2.13.2

PROBLEMS:
 - Spools to a temporary disk
 - Need to specify direct/indirect upload via flag
 - Location of ca.pem
 - Target cluster
 - Delete disks on failure, or rename disks on success?
 - Handling of sparseness in raw format disks

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 by running snippets of Python
code to interact with the ‘ovirtsdk4’ module.  It requires both Python 3
and the Python SDK v4 to be installed at run time (these are not,
however, new dependencies of virt-v2v since most people wouldn't have
them).
---
 v2v/Makefile.am           |   2 +
 v2v/cmdline.ml            |  25 +++
 v2v/output_rhv_upload.ml  | 403 ++++++++++++++++++++++++++++++++++++++++++++++
 v2v/output_rhv_upload.mli |  26 +++
 4 files changed, 456 insertions(+)

diff --git a/v2v/Makefile.am b/v2v/Makefile.am
index 83f0c30c7..c028babe6 100644
--- a/v2v/Makefile.am
+++ b/v2v/Makefile.am
@@ -64,6 +64,7 @@ SOURCES_MLI = \
 	output_null.mli \
 	output_qemu.mli \
 	output_rhv.mli \
+	output_rhv_upload.mli \
 	output_vdsm.mli \
 	parse_ovf_from_ova.mli \
 	parse_libvirt_xml.mli \
@@ -116,6 +117,7 @@ SOURCES_ML = \
 	output_local.ml \
 	output_qemu.ml \
 	output_rhv.ml \
+	output_rhv_upload.ml \
 	output_vdsm.ml \
 	inspect_source.ml \
 	target_bus_assignment.ml \
diff --git a/v2v/cmdline.ml b/v2v/cmdline.ml
index fceda1f82..4424863fe 100644
--- a/v2v/cmdline.ml
+++ b/v2v/cmdline.ml
@@ -138,6 +138,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
     | "qemu" -> output_mode := `QEmu
     | "vdsm" -> output_mode := `VDSM
     | s ->
@@ -537,6 +539,29 @@ read the man page virt-v2v(1).
       Output_rhv.output_rhv os output_alloc,
       output_format, output_alloc
 
+    | `RHV_Upload ->
+      let output_conn +        match output_conn with
+        | None ->
+           error (f_"-o rhv-upload: output connection was not specified,
use ‘-oc’ to point to the oVirt or RHV server REST API URL")
+        | Some oc -> oc in
+      (* In theory we could make the password optional in future. *)
+      let output_password +        match output_password with
+        | None ->
+           error (f_"-o rhv-upload: output password file was not
specified, use ‘-op’ to point to a file which contains the password used to
connect to the oVirt or RHV server")
+        | Some op -> op in
+      let os +        match output_storage with
+        | None ->
+           error (f_"-o rhv-upload: output storage was not specified, use
‘-os’");
+        | Some os -> os in
+      if qemu_boot then
+        error_option_cannot_be_used_in_output_mode "rhv-upload"
"--qemu-boot";
+      Output_rhv_upload.output_rhv_upload output_alloc output_conn
+                                          output_password os,
+      output_format, output_alloc
+
     | `VDSM ->
       if output_password <> None then
         error_option_cannot_be_used_in_output_mode "vdsm"
"-op";
diff --git a/v2v/output_rhv_upload.ml b/v2v/output_rhv_upload.ml
new file mode 100644
index 000000000..77f7bc988
--- /dev/null
+++ b/v2v/output_rhv_upload.ml
@@ -0,0 +1,403 @@
+(* virt-v2v
+ * Copyright (C) 2009-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.
+ *)
+
+open Printf
+open Unix
+
+open Std_utils
+open Tools_utils
+open Unix_utils
+open Common_gettext.Gettext
+
+open Types
+open Utils
+
+(* These correspond mostly to the fields in the Python
+ * sdk.Connection object, except for the password which
+ * is handled separately.
+ *)
+type connection = {
+  conn_url : string;
+  conn_username : string;
+  conn_debug : bool;
+}
+
+let string_of_connection conn +  sprintf "url=%s username=%s
debug=%b"
+          conn.conn_url conn.conn_username conn.conn_debug
+
+(* Python code fragments go first.  Note these must not be
+ * indented because of Python's stupid whitespace thing.
+ *)
+
+(* Print the Python version. *)
+let python_get_version = "
+import sys
+print (sys.version[0]) # syntax works on py2 or py3
+"
+
+(* Import all the Python modules needed. *)
+let python_imports = "
+import logging
+import ovirtsdk4 as sdk
+import ovirtsdk4.types as types
+import ssl
+import sys
+import time
+
+from http.client import HTTPSConnection
+
+try:
+    from urllib.parse import urlparse
+except ImportError:
+    from urlparse import urlparse
+"
+
+(* Create the Python prologue which connects to the system service.
+ * This returns a string of Python code.
+ *)
+let python_connect tmpdir conn output_password +  sprintf "
+password_file = %s
+with open(password_file, 'r') as file:
+    password = file.read()
+password = password.rstrip()
+
+# Open the connection.
+connection = sdk.Connection(
+    url = %s,
+    username = %s,
+    password = password,
+    debug = %s,
+    log = logging.getLogger(),
+    insecure = True,
+)
+system_service = connection.system_service()
+" (py_quote output_password)
+  (py_quote conn.conn_url)
+  (py_quote conn.conn_username)
+  (py_bool conn.conn_debug)
+
+let python_create_one_disk disk_name disk_format
+                           output_alloc output_storage disk_size +  sprintf
"
+disks_service = system_service.disks_service()
+disk = disks_service.add(
+    disk = types.Disk(
+        name = %s,
+        format = %s,
+        sparse = %s,
+        provisioned_size = %Ld,
+        storage_domains = [types.StorageDomain(name = %s)],
+    )
+)
+disk_id = disk.id
+
+# Wait til the disk is up.  The transfer cannot start if the
+# disk is locked.
+disk_service = disks_service.disk_service(disk_id)
+while True:
+    time.sleep(5)
+    disk = disk_service.get()
+    if disk.status == types.DiskStatus.OK:
+        break
+
+# Return the disk ID.
+print(disk_id)
+" (py_quote disk_name)
+  disk_format (* it's a raw Python expression, don't quote *)
+  (py_bool (output_alloc = Sparse))
+  disk_size
+  (py_quote output_storage)
+
+(* XXX Temporary function. *)
+let python_upload_one_disk disk_id disk_size filename +  sprintf "
+transfers_service = system_service.image_transfers_service()
+
+transfer = transfers_service.add(
+    types.ImageTransfer(
+        image = types.Image (id=%s)
+    )
+)
+
+transfer_service = transfers_service.image_transfer_service(transfer.id)
+
+# After adding a new transfer for the disk, the transfer's status will
+# be INITIALIZING.  Wait until the init phase is over.
+while transfer.phase == types.ImageTransferPhase.INITIALIZING:
+    time.sleep(1)
+    transfer = transfer_service.get()
+
+if %s:
+    if transfer.transfer_url is None:
+        print(\"Direct upload to host not supported, requires ovirt-engine
>= 4.2\")
+        sys.exit(1)
+    destination_url = urlparse(transfer.transfer_url)
+else:
+    destination_url = urlparse(transfer.proxy_url)
+
+context = ssl.create_default_context()
+context.load_verify_locations(cafile = %s)
+
+proxy_connection = HTTPSConnection(
+    destination_url.hostname,
+    destination_url.port,
+    context = context
+)
+
+image_path = %s
+image_size = %Ld
+
+proxy_connection.putrequest(\"PUT\", destination_url.path)
+proxy_connection.putheader('Content-Length', image_size)
+proxy_connection.endheaders()
+
+# This seems to give the best throughput when uploading from Yaniv's
+# machine to a server that drops the data. You may need to tune this
+# on your setup.
+BUF_SIZE = 128 * 1024
+
+with open(image_path, \"rb\") as disk:
+    pos = 0
+    while pos < image_size:
+        # Send the next chunk to the proxy.
+        to_read = min(image_size - pos, BUF_SIZE)
+        chunk = disk.read(to_read)
+        if not chunk:
+            transfer_service.pause()
+            raise RuntimeError(\"Unexpected end of file at pos=%%d\"
%% pos)
+
+        proxy_connection.send(chunk)
+        pos += len(chunk)
+
+# Get the response
+response = proxy_connection.getresponse()
+if response.status != 200:
+    transfer_service.pause()
+    print(\"Upload failed: %%s %%s\" %%
+          (response.status, response.reason))
+    sys.exit(1)
+
+# Successful cleanup
+transfer_service.finalize()
+connection.close()
+proxy_connection.close()
+" (py_quote disk_id)
+  (py_bool true(*direct_upload XXX*))
+  (py_quote "/tmp/ca.pem"(*cafile XXX*))
+  (py_quote filename)
+  disk_size
+
+let python_create_virtual_machine ovf +  sprintf "
+vms_service = system_service.vms_service()
+vm = vms_service.add(
+    types.Vm(
+        cluster=types.Cluster(name = %s),
+        initialization=types.Initialization(
+            configuration = types.Configuration(
+                type = types.ConfigurationType.OVA,
+                data = %s
+            )
+        )
+    )
+)
+" (py_quote "Default" (* XXX target cluster *))
+  (py_quote (DOM.doc_to_string ovf))
+
+(* Find the Python 3 binary. *)
+let find_python3 () +  let rec loop = function
+    | [] ->
+       error "could not locate Python 3 binary on the $PATH.  You may have
to install Python 3.  If Python 3 is already installed then you may need to
create a directory containing a binary called ‘python3’ which runs Python
3."
+    | python :: rest ->
+       (* Use shell_command first to check the binary exists. *)
+       let cmd = sprintf "%s --help >/dev/null 2>&1" (quote
python) in
+       if shell_command cmd = 0 &&
+            run_python ~python python_get_version = ["3"] then (
+         debug "rhv-upload: python binary: %s" python;
+         python
+       )
+       else
+         loop rest
+  in
+  loop ["python3"; "python"]
+
+(* Parse the -oc URI. *)
+let parse_output_conn oc +  let uri = Xml.parse_uri oc in
+  if uri.Xml.uri_scheme <> Some "https" then
+    error (f_"rhv-upload: -oc: URI must start with https://...");
+  if uri.Xml.uri_server = None then
+    error (f_"rhv-upload: -oc: no remote server name in the URI");
+  if uri.Xml.uri_path = None || uri.Xml.uri_path = Some "/" then
+    error (f_"rhv-upload: -oc: URI path component looks incorrect");
+  let username +    match uri.Xml.uri_user with
+    | None ->
+       warning (f_"rhv-upload: -oc: username was missing from URI,
assuming ‘admin@internal’");
+       "admin@internal"
+    | Some user -> user in
+  (* Reconstruct the URI without the username. *)
+  let url = sprintf "%s://%s%s"
+                    (Option.default "https" uri.Xml.uri_scheme)
+                    (Option.default "localhost" uri.Xml.uri_server)
+                    (Option.default "" uri.Xml.uri_path) in
+
+  let conn = { conn_url = url; conn_username = username;
+               conn_debug = verbose () } in
+  debug "rhv-upload: connection=%s" (string_of_connection conn);
+  conn
+
+(* Create a single, empty disk on the target. *)
+let create_one_disk run_python tmpdir conn
+                    output_password output_format
+                    output_alloc output_storage
+                    source target +  (* Give the disk a predictable name based
on the source
+   * name and disk index.
+   *)
+  let disk_name +    let id = target.target_overlay.ov_source.s_disk_id in
+    sprintf "%s-%03d" source.s_name id in
+
+  let disk_format +    match output_format with
+    | `Raw -> "types.DiskFormat.RAW"
+    | `COW -> "types.DiskFormat.COW" in
+
+  (* This is the virtual size in bytes. *)
+  let disk_size = target.target_overlay.ov_virtual_size in
+
+  let code +    python_imports ^
+    python_connect tmpdir conn output_password ^
+    python_create_one_disk disk_name disk_format
+                           output_alloc output_storage disk_size in
+  match run_python code with
+  | [id] -> id
+  | _ -> error (f_"rhv-upload: create_one_disk: error creating disks,
see previous output")
+
+(* XXX Temporary function to upload spooled disk. *)
+let upload_one_disk run_python tmpdir conn output_password t filename disk_id +
let disk_size = t.target_overlay.ov_virtual_size in
+
+  let code +    python_imports ^
+    python_connect tmpdir conn output_password ^
+    python_upload_one_disk disk_id disk_size filename in
+  ignore (run_python code)
+
+(* Upload the virtual machine metadata (ie OVF) and create a VM. *)
+let create_virtual_machine run_python tmpdir conn output_password ovf +  let
code +    python_imports ^
+    python_connect tmpdir conn output_password ^
+    python_create_virtual_machine ovf in
+  ignore (run_python code)
+
+class output_rhv_upload output_alloc output_conn
+                        output_password output_storage +  let run_python +   
let python = find_python3 () in
+    run_python ~python in
+  let conn = parse_output_conn output_conn in
+
+  (* The temporary directory is used for a few things such as passing
+   * passwords securely and (temporarily) for spooling disks (XXX).
+   *)
+  let tmpdir +    let base_dir = (open_guestfs ())#get_cachedir () in
+    let t = Mkdtemp.temp_dir ~base_dir "rhvupload." in
+    rmdir_on_exit t;
+    t in
+object
+  inherit output
+
+  method precheck () +    (* Check all the dependencies including the Python 3
oVirt SDK v4
+     * module are installed.  This will fail with a Python error message.
+     *)
+    ignore (run_python python_imports)
+
+  method as_options +    "-o rhv-upload" ^
+    (match output_alloc with
+     | Sparse -> "" (* default, don't need to print it *)
+     | Preallocated -> " -oa preallocated") ^
+    sprintf " -oc %s -op %s -os %s"
+            output_conn output_password output_storage
+
+  method supported_firmware = [ TargetBIOS ]
+
+  (* List of disks we have created.  There will be one per target. *)
+  val mutable target_disk_ids = []
+
+  method prepare_targets source targets +    let targets +      List.map (
+        fun t ->
+          (* Only allow output format "raw" or "qcow2". *)
+          let output_format +            match t.target_format with
+            | "raw" -> `Raw
+            | "qcow2" -> `COW
+            | _ ->
+               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 qcow’ on the command
line.")
+                     t.target_format in
+
+          let disk_id = create_one_disk run_python tmpdir conn output_password
+                                        output_format output_alloc
+                                        output_storage source t in
+
+          (* XXX Temporarily spool disks to tmpdir. *)
+          let target_file = TargetFile (tmpdir // t.target_overlay.ov_sd) in
+          { t with target_file }, disk_id
+      ) targets in
+    target_disk_ids <- List.map snd targets;
+    List.map fst targets
+
+  method create_metadata source targets _ guestcaps inspect target_firmware +  
(* Upload the spooled disks. *)
+    List.iter (
+      fun (t, disk_id) ->
+        let filename +          match t.target_file with
+          | TargetFile filename -> filename
+          | TargetURI _ -> assert false in
+        upload_one_disk run_python tmpdir conn output_password
+                        t filename disk_id
+    ) (List.combine targets target_disk_ids);
+
+    (* Create the metadata. *)
+    let ovf +      Create_ovf.create_ovf source targets guestcaps inspect
+                            Sparse
+                            "domain"
+                            (List.map (fun t -> "") targets)
+                            target_disk_ids
+                            "vm" in
+
+    (* Add the virtual machine. *)
+    create_virtual_machine run_python tmpdir conn output_password ovf
+
+end
+
+let output_rhv_upload = new output_rhv_upload
+let () = Modules_list.register_output_module "rhv-upload"
diff --git a/v2v/output_rhv_upload.mli b/v2v/output_rhv_upload.mli
new file mode 100644
index 000000000..54999b727
--- /dev/null
+++ b/v2v/output_rhv_upload.mli
@@ -0,0 +1,26 @@
+(* virt-v2v
+ * Copyright (C) 2009-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.
+ *)
+
+(** [-o rhv-upload] target. *)
+
+val output_rhv_upload : Types.output_allocation -> string -> string ->
+                        string ->
+                        Types.output
+(** [output_rhv_upload output_alloc output_conn output_password output_storage]
+    creates and returns a new {!Types.output} object specialized for writing
+    output to oVirt or RHV directly via RHV APIs. *)
-- 
2.13.2

On Thu, Feb 22, 2018 at 01:57:25PM +0000, Richard W.M. Jones
wrote:
> PROBLEMS:
>  - Spools to a temporary disk
>  - Need to specify direct/indirect upload via flag
>  - Location of ca.pem
>  - Target cluster
>  - Delete disks on failure, or rename disks on success?
>  - Handling of sparseness in raw format disks
Actually there are a few more problems I forgot about:

- How should disks be named?  Currently it uses <guestname>-000, -001 etc
- How should the OVA file refer to disks?

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-builder quickly builds VMs from scratch
http://libguestfs.org/virt-builder.1.html

On Thu, 22 Feb 2018 13:57:20 +0000
"Richard W.M. Jones" <rjones@redhat.com> wrote:
> The first four patches are straightforward.
> 
> The final patch adds the -o rhv-upload output mode.  It is
> still spooling into a temporary file because I've had some
> trouble getting streaming conversions working.  There are
> other problems as outlined in the commit message, so this
> patch is not ready for upstream but is good for discussion.
> 
> Also I hit this, which I'm assuming for now will be fixed
> by Tomas's work:
> 
>   ovirtsdk4.Error: Fault reason is "Operation Failed". Fault
detail is "failed to parse a given ovf configuration ovf error: [empty
name]: cannot read '//*/disksection' with value: null". HTTP
response code is 400.
Yes, this will be fixed by my OVF series.

    Tomas

-- 
Tomáš Golembiovský <tgolembi@redhat.com>

The previous patches seem OK. On a quick glance this one also seems a
good start.

On Thu, 22 Feb 2018 13:57:25 +0000
"Richard W.M. Jones" <rjones@redhat.com> wrote:
> PROBLEMS:
>  - Spools to a temporary disk
>  - Need to specify direct/indirect upload via flag
>  - Location of ca.pem
We surely need a new argument for that.

When running on VDSM host the certificate is in
'/etc/pki/vdsm/certs/cacert.pem'. Maybe we can use it as default? 
>  - Target cluster
>  - Delete disks on failure, or rename disks on success?
Definitely delete disks on failure. Unless we want to make it
administrators task. If you want to mark disks created by upload maybe
you can put something like 'uploaded by virt-v2v' in a comment? That may
help avoiding any renames... just an idea.

>  - Handling of sparseness in raw format disks
> 
> 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 by running snippets of Python
> code to interact with the ‘ovirtsdk4’ module.  It requires both Python 3
> and the Python SDK v4 to be installed at run time (these are not,
> however, new dependencies of virt-v2v since most people wouldn't have
> them).
> ---
>  v2v/Makefile.am           |   2 +
>  v2v/cmdline.ml            |  25 +++
>  v2v/output_rhv_upload.ml  | 403
++++++++++++++++++++++++++++++++++++++++++++++
>  v2v/output_rhv_upload.mli |  26 +++
>  4 files changed, 456 insertions(+)
> 
> diff --git a/v2v/Makefile.am b/v2v/Makefile.am
> index 83f0c30c7..c028babe6 100644
> --- a/v2v/Makefile.am
> +++ b/v2v/Makefile.am
> @@ -64,6 +64,7 @@ SOURCES_MLI = \
>  	output_null.mli \
>  	output_qemu.mli \
>  	output_rhv.mli \
> +	output_rhv_upload.mli \
>  	output_vdsm.mli \
>  	parse_ovf_from_ova.mli \
>  	parse_libvirt_xml.mli \
> @@ -116,6 +117,7 @@ SOURCES_ML = \
>  	output_local.ml \
>  	output_qemu.ml \
>  	output_rhv.ml \
> +	output_rhv_upload.ml \
>  	output_vdsm.ml \
>  	inspect_source.ml \
>  	target_bus_assignment.ml \
> diff --git a/v2v/cmdline.ml b/v2v/cmdline.ml
> index fceda1f82..4424863fe 100644
> --- a/v2v/cmdline.ml
> +++ b/v2v/cmdline.ml
> @@ -138,6 +138,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
>      | "qemu" -> output_mode := `QEmu
>      | "vdsm" -> output_mode := `VDSM
>      | s ->
> @@ -537,6 +539,29 @@ read the man page virt-v2v(1).
>        Output_rhv.output_rhv os output_alloc,
>        output_format, output_alloc
>  
> +    | `RHV_Upload ->
> +      let output_conn > +        match output_conn with
> +        | None ->
> +           error (f_"-o rhv-upload: output connection was not
specified, use ‘-oc’ to point to the oVirt or RHV server REST API URL")
> +        | Some oc -> oc in
> +      (* In theory we could make the password optional in future. *)
> +      let output_password > +        match output_password with
> +        | None ->
> +           error (f_"-o rhv-upload: output password file was not
specified, use ‘-op’ to point to a file which contains the password used to
connect to the oVirt or RHV server")
> +        | Some op -> op in
> +      let os > +        match output_storage with
> +        | None ->
> +           error (f_"-o rhv-upload: output storage was not specified,
use ‘-os’");
> +        | Some os -> os in
> +      if qemu_boot then
> +        error_option_cannot_be_used_in_output_mode "rhv-upload"
"--qemu-boot";
> +      Output_rhv_upload.output_rhv_upload output_alloc output_conn
> +                                          output_password os,
> +      output_format, output_alloc
> +
>      | `VDSM ->
>        if output_password <> None then
>          error_option_cannot_be_used_in_output_mode "vdsm"
"-op";
> diff --git a/v2v/output_rhv_upload.ml b/v2v/output_rhv_upload.ml
> new file mode 100644
> index 000000000..77f7bc988
> --- /dev/null
> +++ b/v2v/output_rhv_upload.ml
> @@ -0,0 +1,403 @@
> +(* virt-v2v
> + * Copyright (C) 2009-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.
> + *)
> +
> +open Printf
> +open Unix
> +
> +open Std_utils
> +open Tools_utils
> +open Unix_utils
> +open Common_gettext.Gettext
> +
> +open Types
> +open Utils
> +
> +(* These correspond mostly to the fields in the Python
> + * sdk.Connection object, except for the password which
> + * is handled separately.
> + *)
> +type connection = {
> +  conn_url : string;
> +  conn_username : string;
> +  conn_debug : bool;
> +}
> +
> +let string_of_connection conn > +  sprintf "url=%s username=%s
debug=%b"
> +          conn.conn_url conn.conn_username conn.conn_debug
> +
> +(* Python code fragments go first.  Note these must not be
> + * indented because of Python's stupid whitespace thing.
> + *)
> +
> +(* Print the Python version. *)
> +let python_get_version = "
> +import sys
> +print (sys.version[0]) # syntax works on py2 or py3
> +"
> +
> +(* Import all the Python modules needed. *)
> +let python_imports = "
> +import logging
> +import ovirtsdk4 as sdk
> +import ovirtsdk4.types as types
> +import ssl
> +import sys
> +import time
> +
> +from http.client import HTTPSConnection
> +
> +try:
> +    from urllib.parse import urlparse
> +except ImportError:
> +    from urlparse import urlparse
> +"
> +
> +(* Create the Python prologue which connects to the system service.
> + * This returns a string of Python code.
> + *)
> +let python_connect tmpdir conn output_password > +  sprintf "
> +password_file = %s
> +with open(password_file, 'r') as file:
> +    password = file.read()
> +password = password.rstrip()
> +
> +# Open the connection.
> +connection = sdk.Connection(
> +    url = %s,
> +    username = %s,
> +    password = password,
> +    debug = %s,
> +    log = logging.getLogger(),
> +    insecure = True,
> +)
> +system_service = connection.system_service()
> +" (py_quote output_password)
> +  (py_quote conn.conn_url)
> +  (py_quote conn.conn_username)
> +  (py_bool conn.conn_debug)
> +
> +let python_create_one_disk disk_name disk_format
> +                           output_alloc output_storage disk_size > + 
sprintf "
> +disks_service = system_service.disks_service()
> +disk = disks_service.add(
> +    disk = types.Disk(
> +        name = %s,
> +        format = %s,
> +        sparse = %s,
> +        provisioned_size = %Ld,
> +        storage_domains = [types.StorageDomain(name = %s)],
> +    )
> +)
> +disk_id = disk.id
> +
> +# Wait til the disk is up.  The transfer cannot start if the
> +# disk is locked.
> +disk_service = disks_service.disk_service(disk_id)
> +while True:
> +    time.sleep(5)
> +    disk = disk_service.get()
> +    if disk.status == types.DiskStatus.OK:
> +        break
> +
> +# Return the disk ID.
> +print(disk_id)
> +" (py_quote disk_name)
> +  disk_format (* it's a raw Python expression, don't quote *)
> +  (py_bool (output_alloc = Sparse))
> +  disk_size
> +  (py_quote output_storage)
> +
> +(* XXX Temporary function. *)
> +let python_upload_one_disk disk_id disk_size filename > +  sprintf
"
> +transfers_service = system_service.image_transfers_service()
> +
> +transfer = transfers_service.add(
> +    types.ImageTransfer(
> +        image = types.Image (id=%s)
> +    )
> +)
> +
> +transfer_service = transfers_service.image_transfer_service(transfer.id)
> +
> +# After adding a new transfer for the disk, the transfer's status will
> +# be INITIALIZING.  Wait until the init phase is over.
> +while transfer.phase == types.ImageTransferPhase.INITIALIZING:
> +    time.sleep(1)
> +    transfer = transfer_service.get()
> +
> +if %s:
> +    if transfer.transfer_url is None:
> +        print(\"Direct upload to host not supported, requires
ovirt-engine >= 4.2\")
> +        sys.exit(1)
> +    destination_url = urlparse(transfer.transfer_url)
> +else:
> +    destination_url = urlparse(transfer.proxy_url)
> +
> +context = ssl.create_default_context()
> +context.load_verify_locations(cafile = %s)
> +
> +proxy_connection = HTTPSConnection(
> +    destination_url.hostname,
> +    destination_url.port,
> +    context = context
> +)
> +
> +image_path = %s
> +image_size = %Ld
> +
> +proxy_connection.putrequest(\"PUT\", destination_url.path)
> +proxy_connection.putheader('Content-Length', image_size)
> +proxy_connection.endheaders()
> +
> +# This seems to give the best throughput when uploading from Yaniv's
> +# machine to a server that drops the data. You may need to tune this
> +# on your setup.
> +BUF_SIZE = 128 * 1024
> +
> +with open(image_path, \"rb\") as disk:
> +    pos = 0
> +    while pos < image_size:
> +        # Send the next chunk to the proxy.
> +        to_read = min(image_size - pos, BUF_SIZE)
> +        chunk = disk.read(to_read)
> +        if not chunk:
> +            transfer_service.pause()
> +            raise RuntimeError(\"Unexpected end of file at
pos=%%d\" %% pos)
> +
> +        proxy_connection.send(chunk)
> +        pos += len(chunk)
> +
> +# Get the response
> +response = proxy_connection.getresponse()
> +if response.status != 200:
> +    transfer_service.pause()
> +    print(\"Upload failed: %%s %%s\" %%
> +          (response.status, response.reason))
> +    sys.exit(1)
> +
> +# Successful cleanup
> +transfer_service.finalize()
> +connection.close()
> +proxy_connection.close()
> +" (py_quote disk_id)
> +  (py_bool true(*direct_upload XXX*))
> +  (py_quote "/tmp/ca.pem"(*cafile XXX*))
> +  (py_quote filename)
> +  disk_size
> +
> +let python_create_virtual_machine ovf > +  sprintf "
> +vms_service = system_service.vms_service()
> +vm = vms_service.add(
> +    types.Vm(
> +        cluster=types.Cluster(name = %s),
> +        initialization=types.Initialization(
> +            configuration = types.Configuration(
> +                type = types.ConfigurationType.OVA,
> +                data = %s
> +            )
> +        )
> +    )
> +)
> +" (py_quote "Default" (* XXX target cluster *))
> +  (py_quote (DOM.doc_to_string ovf))
> +
> +(* Find the Python 3 binary. *)
> +let find_python3 () > +  let rec loop = function
> +    | [] ->
> +       error "could not locate Python 3 binary on the $PATH.  You may
have to install Python 3.  If Python 3 is already installed then you may need to
create a directory containing a binary called ‘python3’ which runs Python
3."
> +    | python :: rest ->
> +       (* Use shell_command first to check the binary exists. *)
> +       let cmd = sprintf "%s --help >/dev/null 2>&1"
(quote python) in
> +       if shell_command cmd = 0 &&
> +            run_python ~python python_get_version = ["3"] then (
> +         debug "rhv-upload: python binary: %s" python;
> +         python
> +       )
> +       else
> +         loop rest
> +  in
> +  loop ["python3"; "python"]
> +
> +(* Parse the -oc URI. *)
> +let parse_output_conn oc > +  let uri = Xml.parse_uri oc in
> +  if uri.Xml.uri_scheme <> Some "https" then
> +    error (f_"rhv-upload: -oc: URI must start with
https://...");
> +  if uri.Xml.uri_server = None then
> +    error (f_"rhv-upload: -oc: no remote server name in the
URI");
> +  if uri.Xml.uri_path = None || uri.Xml.uri_path = Some "/" then
> +    error (f_"rhv-upload: -oc: URI path component looks
incorrect");
> +  let username > +    match uri.Xml.uri_user with
> +    | None ->
> +       warning (f_"rhv-upload: -oc: username was missing from URI,
assuming ‘admin@internal’");
> +       "admin@internal"
> +    | Some user -> user in
I assume quoting the @ in URL works here and url like this:

    https://admin%40internal@my-ovirt.com/ovirt-engine/api

will be parsed properly.

    Tomas
> +  (* Reconstruct the URI without the username. *)
> +  let url = sprintf "%s://%s%s"
> +                    (Option.default "https" uri.Xml.uri_scheme)
> +                    (Option.default "localhost"
uri.Xml.uri_server)
> +                    (Option.default "" uri.Xml.uri_path) in
> +
> +  let conn = { conn_url = url; conn_username = username;
> +               conn_debug = verbose () } in
> +  debug "rhv-upload: connection=%s" (string_of_connection conn);
> +  conn
> +
> +(* Create a single, empty disk on the target. *)
> +let create_one_disk run_python tmpdir conn
> +                    output_password output_format
> +                    output_alloc output_storage
> +                    source target > +  (* Give the disk a predictable
name based on the source
> +   * name and disk index.
> +   *)
> +  let disk_name > +    let id =
target.target_overlay.ov_source.s_disk_id in
> +    sprintf "%s-%03d" source.s_name id in
> +
> +  let disk_format > +    match output_format with
> +    | `Raw -> "types.DiskFormat.RAW"
> +    | `COW -> "types.DiskFormat.COW" in
> +
> +  (* This is the virtual size in bytes. *)
> +  let disk_size = target.target_overlay.ov_virtual_size in
> +
> +  let code > +    python_imports ^
> +    python_connect tmpdir conn output_password ^
> +    python_create_one_disk disk_name disk_format
> +                           output_alloc output_storage disk_size in
> +  match run_python code with
> +  | [id] -> id
> +  | _ -> error (f_"rhv-upload: create_one_disk: error creating
disks, see previous output")
> +
> +(* XXX Temporary function to upload spooled disk. *)
> +let upload_one_disk run_python tmpdir conn output_password t filename
disk_id > +  let disk_size = t.target_overlay.ov_virtual_size in
> +
> +  let code > +    python_imports ^
> +    python_connect tmpdir conn output_password ^
> +    python_upload_one_disk disk_id disk_size filename in
> +  ignore (run_python code)
> +
> +(* Upload the virtual machine metadata (ie OVF) and create a VM. *)
> +let create_virtual_machine run_python tmpdir conn output_password ovf >
+  let code > +    python_imports ^
> +    python_connect tmpdir conn output_password ^
> +    python_create_virtual_machine ovf in
> +  ignore (run_python code)
> +
> +class output_rhv_upload output_alloc output_conn
> +                        output_password output_storage > +  let
run_python > +    let python = find_python3 () in
> +    run_python ~python in
> +  let conn = parse_output_conn output_conn in
> +
> +  (* The temporary directory is used for a few things such as passing
> +   * passwords securely and (temporarily) for spooling disks (XXX).
> +   *)
> +  let tmpdir > +    let base_dir = (open_guestfs ())#get_cachedir () in
> +    let t = Mkdtemp.temp_dir ~base_dir "rhvupload." in
> +    rmdir_on_exit t;
> +    t in
> +object
> +  inherit output
> +
> +  method precheck () > +    (* Check all the dependencies including the
Python 3 oVirt SDK v4
> +     * module are installed.  This will fail with a Python error message.
> +     *)
> +    ignore (run_python python_imports)
> +
> +  method as_options > +    "-o rhv-upload" ^
> +    (match output_alloc with
> +     | Sparse -> "" (* default, don't need to print it *)
> +     | Preallocated -> " -oa preallocated") ^
> +    sprintf " -oc %s -op %s -os %s"
> +            output_conn output_password output_storage
> +
> +  method supported_firmware = [ TargetBIOS ]
> +
> +  (* List of disks we have created.  There will be one per target. *)
> +  val mutable target_disk_ids = []
> +
> +  method prepare_targets source targets > +    let targets > +     
List.map (
> +        fun t ->
> +          (* Only allow output format "raw" or
"qcow2". *)
> +          let output_format > +            match t.target_format with
> +            | "raw" -> `Raw
> +            | "qcow2" -> `COW
> +            | _ ->
> +               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 qcow’ on the command
line.")
> +                     t.target_format in
> +
> +          let disk_id = create_one_disk run_python tmpdir conn
output_password
> +                                        output_format output_alloc
> +                                        output_storage source t in
> +
> +          (* XXX Temporarily spool disks to tmpdir. *)
> +          let target_file = TargetFile (tmpdir // t.target_overlay.ov_sd)
in
> +          { t with target_file }, disk_id
> +      ) targets in
> +    target_disk_ids <- List.map snd targets;
> +    List.map fst targets
> +
> +  method create_metadata source targets _ guestcaps inspect
target_firmware > +    (* Upload the spooled disks. *)
> +    List.iter (
> +      fun (t, disk_id) ->
> +        let filename > +          match t.target_file with
> +          | TargetFile filename -> filename
> +          | TargetURI _ -> assert false in
> +        upload_one_disk run_python tmpdir conn output_password
> +                        t filename disk_id
> +    ) (List.combine targets target_disk_ids);
> +
> +    (* Create the metadata. *)
> +    let ovf > +      Create_ovf.create_ovf source targets guestcaps
inspect
> +                            Sparse
> +                            "domain"
> +                            (List.map (fun t -> "") targets)
> +                            target_disk_ids
> +                            "vm" in
> +
> +    (* Add the virtual machine. *)
> +    create_virtual_machine run_python tmpdir conn output_password ovf
> +
> +end
> +
> +let output_rhv_upload = new output_rhv_upload
> +let () = Modules_list.register_output_module "rhv-upload"
> diff --git a/v2v/output_rhv_upload.mli b/v2v/output_rhv_upload.mli
> new file mode 100644
> index 000000000..54999b727
> --- /dev/null
> +++ b/v2v/output_rhv_upload.mli
> @@ -0,0 +1,26 @@
> +(* virt-v2v
> + * Copyright (C) 2009-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.
> + *)
> +
> +(** [-o rhv-upload] target. *)
> +
> +val output_rhv_upload : Types.output_allocation -> string -> string
->
> +                        string ->
> +                        Types.output
> +(** [output_rhv_upload output_alloc output_conn output_password
output_storage]
> +    creates and returns a new {!Types.output} object specialized for
writing
> +    output to oVirt or RHV directly via RHV APIs. *)
> -- 
> 2.13.2
> 

-- 
Tomáš Golembiovský <tgolembi@redhat.com>

On Thursday, 22 February 2018 14:57:21 CET Richard W.M. Jones
wrote:
> +and buffer_add_spaces buf n > +  for i = 0 to n-1 do Buffer.add_char
buf ' ' done
Maybe String.spaces could make this easier, e.g.

  and buffer_add_spaces buf n     Buffer.add_string buf (String.spaces n)

The rest LGTM.

-- 
Pino Toscano

On Thursday, 22 February 2018 14:57:25 CET Richard W.M. Jones
wrote:
> PROBLEMS:
>  - Spools to a temporary disk
>  - Need to specify direct/indirect upload via flag
>  - Location of ca.pem
>  - Target cluster
>  - Delete disks on failure, or rename disks on success?
>  - Handling of sparseness in raw format disks
> 
> 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 by running snippets of Python
> code to interact with the ‘ovirtsdk4’ module.  It requires both Python 3
> and the Python SDK v4 to be installed at run time (these are not,
> however, new dependencies of virt-v2v since most people wouldn't have
> them).
> ---
Since the patch is still RFC, and I cannot comment on the actual
procedure, I'll just leave few misc comments.
> +    | `RHV_Upload ->
> +      let output_conn > +        match output_conn with
> +        | None ->
> +           error (f_"-o rhv-upload: output connection was not
specified, use ‘-oc’ to point to the oVirt or RHV server REST API URL")
More than "output connection", I'd just mention "REST API
URL"
directly.
> +# Wait til the disk is up.  The transfer cannot start if the
> +# disk is locked.
> +disk_service = disks_service.disk_service(disk_id)
> +while True:
> +    time.sleep(5)
> +    disk = disk_service.get()
> +    if disk.status == types.DiskStatus.OK:
> +        break
Is a busy loop the only way to wait for an OK disk?

Regardless, this (and other busy loops in the Python snippets) IMHO
need also a timeout, otherwise any error coming from oVirt will block
v2v indefinitely...
> +# This seems to give the best throughput when uploading from Yaniv's
> +# machine to a server that drops the data. You may need to tune this
> +# on your setup.
> +BUF_SIZE = 128 * 1024
> +
> +with open(image_path, \"rb\") as disk:
> +    pos = 0
> +    while pos < image_size:
> +        # Send the next chunk to the proxy.
> +        to_read = min(image_size - pos, BUF_SIZE)
> +        chunk = disk.read(to_read)
> +        if not chunk:
> +            transfer_service.pause()
> +            raise RuntimeError(\"Unexpected end of file at
pos=%%d\" %% pos)
> +
> +        proxy_connection.send(chunk)
> +        pos += len(chunk)
No need for 'pos', just use disk.tell() to get the current position
in the file being read.

Also, I'd flip it to the other side: instead of count the read bytes
until the file size, just keep read & send bytes, and error out when
reading more data than the file size.

Maybe something like the untested:

  with open(image_path, "rb") as disk:
      while True:
          # Send the next chunk to the proxy.
          chunk = disk.read(BUF_SIZE)
          if not chunk:
              transfer_service.pause()
              raise RuntimeError("Unexpected end of file at pos=%d" %
file.tell())

          if disk.tell() > image_size:
              transfer_service.pause()
              # maybe even stat() image_path, and show more details in the
exception message
              raise RuntimeError("Read more data than expected: pos=%d vs
size=%d" % file.tell(), image_size)

          proxy_connection.send(chunk)
> +# Get the response
> +response = proxy_connection.getresponse()
> +if response.status != 200:
> +    transfer_service.pause()
> +    print(\"Upload failed: %%s %%s\" %%
> +          (response.status, response.reason))
> +    sys.exit(1)
sys.exit() vs raise?
> +(* Find the Python 3 binary. *)
> +let find_python3 () > +  let rec loop = function
> +    | [] ->
> +       error "could not locate Python 3 binary on the $PATH.  You may
have to install Python 3.  If Python 3 is already installed then you may need to
create a directory containing a binary called ‘python3’ which runs Python
3."
> +    | python :: rest ->
> +       (* Use shell_command first to check the binary exists. *)
> +       let cmd = sprintf "%s --help >/dev/null 2>&1"
(quote python) in
Std_utils.which here fits better IMHO.  If the binary is broken, then
the run_python after this will fail anyway.
> +(* Parse the -oc URI. *)
> +let parse_output_conn oc > +  let uri = Xml.parse_uri oc in
> +  if uri.Xml.uri_scheme <> Some "https" then
> +    error (f_"rhv-upload: -oc: URI must start with
https://...");
> +  if uri.Xml.uri_server = None then
> +    error (f_"rhv-upload: -oc: no remote server name in the
URI");
> +  if uri.Xml.uri_path = None || uri.Xml.uri_path = Some "/" then
> +    error (f_"rhv-upload: -oc: URI path component looks
incorrect");
> +  let username > +    match uri.Xml.uri_user with
> +    | None ->
> +       warning (f_"rhv-upload: -oc: username was missing from URI,
assuming ‘admin@internal’");
> +       "admin@internal"
> +    | Some user -> user in
> +  (* Reconstruct the URI without the username. *)
> +  let url = sprintf "%s://%s%s"
> +                    (Option.default "https" uri.Xml.uri_scheme)
> +                    (Option.default "localhost"
uri.Xml.uri_server)
> +                    (Option.default "" uri.Xml.uri_path) in
Since the checks above make sure uri.Xml.uri_server is not None, then
I'd use "invalid-hostname" or something like that instead of
localhost,
just as safety measure.
> +               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 qcow’ on the command
line.")
Typo, "qcow" -> "qcow2".
> +    (* Create the metadata. *)
> +    let ovf > +      Create_ovf.create_ovf source targets guestcaps
inspect
> +                            Sparse
> +                            "domain"
> +                            (List.map (fun t -> "") targets)
> +                            target_disk_ids
> +                            "vm" in
Shouldn't real UUIDs (as in random UUIDs) be used instead of
"domain",
and "vm"?

-- 
Pino Toscano

On Thursday, 22 February 2018 14:57:20 CET Richard W.M. Jones
wrote:
> The first four patches are straightforward.
I'd push patch #1 (with the change suggested, in case), and #3, as they
are generic enough.

-- 
Pino Toscano