[Libguestfs] [PATCH v2v v2] output: -o rhv-upload: Kill nbdkit instances before finalizing

[Laszlo Ersek]

On 02/09/22 11:32, Richard W.M. Jones wrote:
> If nbdkit instance(s) are still running then they will hold open some
> http connections to imageio.  In some versions of imageio, starting
> finalization in this state causes a 60 second timeout.
> 
> See-also: https://listman.redhat.com/archives/libguestfs/2022-February/msg00111.html
> Thanks: Nir Soffer
> ---
>  output/output_rhv_upload.ml | 39 ++++++++++++++++++++++++++++---------
>  1 file changed, 30 insertions(+), 9 deletions(-)
> 
> diff --git a/output/output_rhv_upload.ml b/output/output_rhv_upload.ml
> index 4d8dc1c135..b500551c5f 100644
> --- a/output/output_rhv_upload.ml
> +++ b/output/output_rhv_upload.ml
> @@ -280,9 +280,22 @@ e command line has to match the number of guest disk images (for this guest: %d)
>      ignore (Python_script.run_command cancel_script json_params [])
>    in
>  
> -  (* Set up an at-exit handler so we delete the orphan disks on failure. *)
> +  (* Set up an at-exit handler to perform some cleanups.
> +   * - Kill nbdkit PIDs (only before finalization).
> +   * - Delete the orphan disks (only on conversion failure).
> +   *)
> +  let nbdkit_pids = ref [] in
>    On_exit.f (
>      fun () ->
> +      (* Kill the nbdkit PIDs. *)
> +      List.iter (
> +        fun pid ->
> +          try
> +            kill pid Sys.sigterm
> +          with exn -> debug "%s" (Printexc.to_string exn)
> +      ) !nbdkit_pids;
> +      nbdkit_pids := [];
> +
>        (* virt-v2v writes v2vdir/done on success only. *)
>        let success = Sys.file_exists (dir // "done") in
>        if not success then (
> @@ -351,11 +364,7 @@ e command line has to match the number of guest disk images (for this guest: %d)
>        if is_ovirt_host then
>          Nbdkit.add_arg cmd "is_ovirt_host" "true";
>        let _, pid = Nbdkit.run_unix ~socket cmd in
> -
> -      (* --exit-with-parent should ensure nbdkit is cleaned
> -       * up when we exit, but it's not supported everywhere.
> -       *)
> -      On_exit.kill pid;
> +      List.push_front pid nbdkit_pids
>    ) (List.combine disks disk_uuids);
>  
>    (* Stash some data we will need during finalization. *)
> @@ -363,7 +372,7 @@ e command line has to match the number of guest disk images (for this guest: %d)
>    let t = (disk_sizes : int64 list), disk_uuids, !transfer_ids,
>            finalize_script, createvm_script, json_params,
>            rhv_storagedomain_uuid, rhv_cluster_uuid,
> -          rhv_cluster_cpu_architecture, rhv_cluster_name in
> +          rhv_cluster_cpu_architecture, rhv_cluster_name, nbdkit_pids in
>    t
>  
>  and rhv_upload_finalize dir source inspect target_meta
> @@ -374,7 +383,8 @@ and rhv_upload_finalize dir source inspect target_meta
>                          (disk_sizes, disk_uuids, transfer_ids,
>                           finalize_script, createvm_script, json_params,
>                           rhv_storagedomain_uuid, rhv_cluster_uuid,
> -                         rhv_cluster_cpu_architecture, rhv_cluster_name) =
> +                         rhv_cluster_cpu_architecture, rhv_cluster_name,
> +                         nbdkit_pids) =
>    (* Check the cluster CPU arch matches what we derived about the
>     * guest during conversion.
>     *)
> @@ -386,6 +396,17 @@ and rhv_upload_finalize dir source inspect target_meta
>            rhv_cluster_name target_meta.guestcaps.gcaps_arch arch
>    );
>  
> +  (* We must kill all our nbdkit instances before finalizing the
> +   * transfer.  See:
> +   * https://listman.redhat.com/archives/libguestfs/2022-February/msg00111.html
> +   *
> +   * We want to fail here if the kill fails because nbdkit
> +   * died already, as that would be unexpected.
> +   *)
> +  List.iter (fun pid -> kill pid Sys.sigterm) !nbdkit_pids;
> +  List.iter (fun pid -> ignore (waitpid [] pid)) !nbdkit_pids;
> +  nbdkit_pids := []; (* Don't kill them again in the On_exit handler. *)
> +
>    (* Finalize all the transfers. *)
>    let json_params =
>      let ids = List.map (fun id -> JSON.String id) transfer_ids in
> @@ -442,7 +463,7 @@ module RHVUpload = struct
>    type t = int64 list * string list * string list *
>             Python_script.script * Python_script.script *
>             JSON.field list * string option * string option *
> -           string option * string
> +           string option * string * int list ref
>  
>    let to_string options =
>      "-o rhv-upload" ^
> 

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