From 96efdcf54c887ae88d54332df12a5f5dd962fd0a Mon Sep 17 00:00:00 2001

From: "Richard W.M. Jones" <rjones@redhat.com>

Date: Fri, 15 Jul 2022 11:25:45 +0100

Subject: [PATCH] output: Permit output modes to wait on the local NBD server

Output.output_to_local_file is used by several output modes that write

to local files or devices.  It launches an instance of qemu-nbd or

nbdkit connected to the local file.

Previously we unconditionally added an On_exit handler to kill the NBD

server.  This is usually safe because nbdcopy --flush has guaranteed

that the data was written through to permanent storage, and so killing

the NBD server is just there to prevent orphaned processes.

However for output to RHV (-o rhv) we actually need the NBD server to

be cleaned up before we exit.  See the analysis here:

https://bugzilla.redhat.com/show_bug.cgi?id=1953286#c26

Allow an alternate strategy of waiting for the NBD server to exit

during virt-v2v shutdown.

We only need this in virt-v2v so implement it here instead of pushing

it all the way into the On_exit module.

Reviewed-by: Laszlo Ersek <lersek@redhat.com>

(cherry picked from commit e2a1a7b4dfb6a9e44260da10a7e7029c09753b5c)

---

 output/output.ml  | 91 ++++++++++++++++++++++++++++-------------------

 output/output.mli | 17 +++++++--

 2 files changed, 69 insertions(+), 39 deletions(-)

diff --git a/output/output.ml b/output/output.ml

index 496c32b6..8f83a324 100644

--- a/output/output.ml

+++ b/output/output.ml

@@ -69,7 +69,10 @@ let error_if_disk_count_gt dir n =

   if Sys.file_exists socket then

     error (f_"this output module doesn't support copying more than %d disks") n

+type on_exit_kill = Kill | KillAndWait

+

 let output_to_local_file ?(changeuid = fun f -> f ()) ?(compressed = false)

+      ?(on_exit_kill = Kill)

       output_alloc output_format filename size socket =

   (* Check nbdkit is installed and has the required plugin. *)

   if not (Nbdkit.is_installed ()) then

@@ -94,46 +97,60 @@ let output_to_local_file ?(changeuid = fun f -> f ()) ?(compressed = false)

     fun () -> g#disk_create ?preallocation filename output_format size

   );

-  match output_format with

-  | "raw" ->

-     let cmd = Nbdkit.create "file" in

-     Nbdkit.add_arg cmd "file" filename;

-     if Nbdkit.version nbdkit_config >= (1, 22, 0) then (

-       let cmd = Nbdkit.add_arg cmd "cache" "none" in

-       cmd

-     );

-     let _, pid = Nbdkit.run_unix socket cmd in

+  let pid =

+    match output_format with

+    | "raw" ->

+       let cmd = Nbdkit.create "file" in

+       Nbdkit.add_arg cmd "file" filename;

+       if Nbdkit.version nbdkit_config >= (1, 22, 0) then (

+         let cmd = Nbdkit.add_arg cmd "cache" "none" in

+         cmd

+       );

+       let _, pid = Nbdkit.run_unix socket cmd in

+       pid

-     (* --exit-with-parent should ensure nbdkit is cleaned

-      * up when we exit, but it's not supported everywhere.

-      *)

-     On_exit.kill pid

+    | "qcow2" ->

+       let cmd =

+         if compressed then (

+           let qemu_quote str = String.replace str "," ",," in

+           let image_opts = [ "driver=compress";

+                              "file.driver=qcow2";

+                              "file.file.driver=file";

+                              "file.file.filename=" ^ qemu_quote filename ] in

+           let image_opts = String.concat "," image_opts in

+           let cmd = QemuNBD.create image_opts in

+           QemuNBD.set_image_opts cmd true;

+           cmd

+         )

+         else (* not compressed *) (

+           let cmd = QemuNBD.create filename in

+           QemuNBD.set_format cmd (Some "qcow2");

+           cmd

+         ) in

+       QemuNBD.set_snapshot cmd false;

+       let _, pid = QemuNBD.run_unix socket cmd in

+       pid

-  | "qcow2" ->

-     let cmd =

-       if compressed then (

-         let qemu_quote str = String.replace str "," ",," in

-         let image_opts = [ "driver=compress";

-                            "file.driver=qcow2";

-                            "file.file.driver=file";

-                            "file.file.filename=" ^ qemu_quote filename ] in

-         let image_opts = String.concat "," image_opts in

-         let cmd = QemuNBD.create image_opts in

-         QemuNBD.set_image_opts cmd true;

-         cmd

-       )

-       else (* not compressed *) (

-         let cmd = QemuNBD.create filename in

-         QemuNBD.set_format cmd (Some "qcow2");

-         cmd

-       ) in

-     QemuNBD.set_snapshot cmd false;

-     let _, pid = QemuNBD.run_unix socket cmd in

-     On_exit.kill pid

+    | _ ->

+       error (f_"output mode only supports raw or qcow2 format (format: %s)")

+         output_format in

+

+  match on_exit_kill with

+  | Kill ->

+    (* Kill the NBD server on exit.  (For nbdkit we use --exit-with-parent

+     * but it's not supported everywhere).

+     *)

+    On_exit.kill pid

-  | _ ->

-     error (f_"output mode only supports raw or qcow2 format (format: %s)")

-       output_format

+  | KillAndWait ->

+     On_exit.f (

+       fun () ->

+         kill pid Sys.sigterm;

+         (* Errors from the NBD server don't matter.  On successful

+          * completion we've already committed the data to disk.

+          *)

+         ignore (waitpid [] pid)

+     )

 let disk_path os name i =

   let outdisk = sprintf "%s/%s-sd%s" os name (drive_name i) in

diff --git a/output/output.mli b/output/output.mli

index c1f0f53d..c4486311 100644

--- a/output/output.mli

+++ b/output/output.mli

@@ -83,14 +83,27 @@ val error_if_disk_count_gt : string -> int -> unit

     "in[n]" in the v2v directory [dir].  If the socket exists, [error] is

     called. *)

+type on_exit_kill = Kill | KillAndWait

+

 val output_to_local_file : ?changeuid:((unit -> unit) -> unit) ->

-                           ?compressed:bool ->

+                           ?compressed:bool -> ?on_exit_kill:on_exit_kill ->

                            Types.output_allocation ->

                            string -> string -> int64 -> string ->

                            unit

 (** When an output mode wants to create a local file with a

     particular format (only "raw" or "qcow2" allowed) then

-    this common function can be used. *)

+    this common function can be used.

+

+    Optional parameter [?on_exit_kill] controls how the NBD server

+    is cleaned up.  The default is {!Kill} which registers an

+    {!On_exit.kill} handler that kills (but does not wait for)

+    the server when virt-v2v exits.  Most callers should use this.

+

+    Setting [~on_exit_kill:KillAndWait] should be used if the NBD

+    server must fully exit before we continue with the rest of

+    virt-v2v shut down.  This is only necessary if some other action

+    (such as unmounting a host filesystem or removing a host device)

+    depends on the NBD server releasing resources. *)

 val disk_path : string -> string -> int -> string

 (** For [-o disk|qemu], return the output disk name of the i'th disk,