Libguestfs - Jul 2022 - [PATCH virt-v2v v3 2/2] -o rhv: Wait for the NBD server to exit to avoid a race with unmounting

Less churn version as described here:
https://listman.redhat.com/archives/libguestfs/2022-July/029483.html

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.
---
 output/output.mli | 17 +++++++--
 output/output.ml  | 91 ++++++++++++++++++++++++++++-------------------
 2 files changed, 69 insertions(+), 39 deletions(-)

diff --git a/output/output.mli b/output/output.mli
index c1f0f53d8e..c448631189 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,
diff --git a/output/output.ml b/output/output.ml
index 23c3932d1b..6065e5923d 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
@@ -105,46 +108,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
-- 
2.37.0.rc2

https://bugzilla.redhat.com/show_bug.cgi?id=1953286#c26
---
 output/output_rhv.ml | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/output/output_rhv.ml b/output/output_rhv.ml
index 15a2c14adf..45f831e39a 100644
--- a/output/output_rhv.ml
+++ b/output/output_rhv.ml
@@ -175,7 +175,14 @@ module RHV = struct
               chmod filename 0o666
           )
         in
-        output_to_local_file ~changeuid
+
+        (* We have to wait for the NBD server to exit rather than just
+         * killing it, otherwise it races with unmounting.  See:
+         * https://bugzilla.redhat.com/show_bug.cgi?id=1953286#c26
+         *)
+        let on_exit_kill = Output.KillAndWait in
+
+        output_to_local_file ~changeuid ~on_exit_kill
           output_alloc output_format filename size socket
     ) (List.combine disks filenames);
 
-- 
2.37.0.rc2

On 07/15/22 14:11, Richard W.M. Jones wrote:
> Less churn version as described here:
> https://listman.redhat.com/archives/libguestfs/2022-July/029483.html
> 
> 
Couple of confusing points here, but I think I sorted them all out:

- the cover letter says 2/2 instead of 0/2

- it wasn't clear to me that the final patch from the common series,
"[PATCH common 4/4] mltools: Allow waiting for killed PIDs", had not
been pushed (because now that patch would be superfluous)

- "[PATCH virt-v2v v3 1/2] output: Permit output modes to wait on the
local NBD server" is quite difficult to review without "-b", due
to the
reindentation; however, after I applied it locally, it was quite reviewable.

- The idea to implement kill+wait with a one-off callback function in
virt-v2v, rather than pushing kill+wait down to libguestfs-common's
mltools/on_exit, is nice (we only really need this in virt-v2v, and
there we do centralize it), but the conceptual jump wasn't written up
anywhere (... anywhere I could see anyway), and I kind of had to deduce
the intent from the code... Such big changes are best put in commit
message changelogs or the cover letters.

Long story short:

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

Thanks!
Laszlo