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 the On_exit handler to be overridden for output modes (only -o rhv) which wish to handle this by themselves. --- output/output.mli | 16 ++++++-- output/output.ml | 79 ++++++++++++++++++++------------------ output/output_disk.ml | 4 +- output/output_glance.ml | 2 +- output/output_libvirt.ml | 4 +- output/output_openstack.ml | 2 +- output/output_qemu.ml | 4 +- output/output_rhv.ml | 4 +- output/output_vdsm.ml | 3 +- 9 files changed, 67 insertions(+), 51 deletions(-) diff --git a/output/output.mli b/output/output.mli index c1f0f53d8e..5eda03c74b 100644 --- a/output/output.mli +++ b/output/output.mli @@ -84,13 +84,23 @@ val error_if_disk_count_gt : string -> int -> unit called. *) val output_to_local_file : ?changeuid:((unit -> unit) -> unit) -> - ?compressed:bool -> + ?compressed:bool -> ?on_exit_kill:bool -> Types.output_allocation -> string -> string -> int64 -> string -> - unit + int (** 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. + + Exit handling: + + [?on_exit_kill] defaults to true. Most callers can leave that alone, + and also ignore the returned PID. The NBD server will be cleaned up + automatically with no further action required. + + However if you need to wait for the NBD server to be cleaned up, + you should set [~on_exit_kill:false] and then waitpid on the + returned PID. *) 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..0e57c4c7f6 100644 --- a/output/output.ml +++ b/output/output.ml @@ -70,6 +70,7 @@ let error_if_disk_count_gt dir n = error (f_"this output module doesn't support copying more than %d disks") n let output_to_local_file ?(changeuid = fun f -> f ()) ?(compressed = false) + ?(on_exit_kill = true) output_alloc output_format filename size socket = (* Check nbdkit is installed and has the required plugin. *) if not (Nbdkit.is_installed ()) then @@ -105,46 +106,50 @@ 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 - | _ -> - error (f_"output mode only supports raw or qcow2 format (format: %s)") - output_format + if on_exit_kill then + (* Kill the NBD server on exit. (For nbdkit we use --exit-with-parent + * but it's not supported everywhere). + *) + On_exit.kill pid; + 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_disk.ml b/output/output_disk.ml index abcfcdc0ce..beef22011b 100644 --- a/output/output_disk.ml +++ b/output/output_disk.ml @@ -85,8 +85,8 @@ module Disk = struct (* Create the actual output disk. *) let outdisk = disk_path output_storage output_name i in - output_to_local_file ~compressed output_alloc output_format - outdisk size socket + ignore (output_to_local_file ~compressed output_alloc output_format + outdisk size socket) ) disks let finalize dir options () source inspect target_meta = diff --git a/output/output_glance.ml b/output/output_glance.ml index e6a7f78994..40c6bb6993 100644 --- a/output/output_glance.ml +++ b/output/output_glance.ml @@ -85,7 +85,7 @@ module Glance = struct (* Create the actual output disk. *) let outdisk = sprintf "%s/%d" tmpdir i in - output_to_local_file Sparse output_format outdisk size socket + ignore (output_to_local_file Sparse output_format outdisk size socket) ) disks; tmpdir diff --git a/output/output_libvirt.ml b/output/output_libvirt.ml index 04b4c5f81e..1188d560d7 100644 --- a/output/output_libvirt.ml +++ b/output/output_libvirt.ml @@ -130,8 +130,8 @@ module Libvirt_ = struct (* Create the actual output disk. *) let outdisk = target_path // output_name ^ "-sd" ^ (drive_name i) in - output_to_local_file ~compressed output_alloc output_format - outdisk size socket + ignore (output_to_local_file ~compressed output_alloc output_format + outdisk size socket) ) disks; (capabilities_xml, pool_name) diff --git a/output/output_openstack.ml b/output/output_openstack.ml index aa01d5a67c..bbfe53c741 100644 --- a/output/output_openstack.ml +++ b/output/output_openstack.ml @@ -380,7 +380,7 @@ The os-* parameters and environment variables are optional. let socket = sprintf "%s/out%d" dir i in On_exit.unlink socket; - output_to_local_file Sparse "raw" dev size socket + ignore (output_to_local_file Sparse "raw" dev size socket) ) (List.combine disks devices); !volume_ids diff --git a/output/output_qemu.ml b/output/output_qemu.ml index 5788fc4255..b1eb3e9cd0 100644 --- a/output/output_qemu.ml +++ b/output/output_qemu.ml @@ -91,8 +91,8 @@ module QEMU = struct (* Create the actual output disk. *) let outdisk = disk_path output_storage output_name i in - output_to_local_file ~compressed output_alloc output_format - outdisk size socket + ignore (output_to_local_file ~compressed output_alloc output_format + outdisk size socket) ) disks let finalize dir options () source inspect target_meta = diff --git a/output/output_rhv.ml b/output/output_rhv.ml index 15a2c14adf..5e3c031e38 100644 --- a/output/output_rhv.ml +++ b/output/output_rhv.ml @@ -175,8 +175,8 @@ module RHV = struct chmod filename 0o666 ) in - output_to_local_file ~changeuid - output_alloc output_format filename size socket + ignore (output_to_local_file ~changeuid + output_alloc output_format filename size socket) ) (List.combine disks filenames); (* Save parameters since we need them during finalization. *) diff --git a/output/output_vdsm.ml b/output/output_vdsm.ml index 23d1b9cd25..c8da47c7c1 100644 --- a/output/output_vdsm.ml +++ b/output/output_vdsm.ml @@ -200,7 +200,8 @@ For each disk you must supply one of each of these options: On_exit.unlink socket; (* Create the actual output disk. *) - output_to_local_file output_alloc output_format filename size socket + ignore (output_to_local_file output_alloc output_format + filename size socket) ) (List.combine disks filenames); (* Save parameters since we need them during finalization. *) -- 2.37.0.rc2

https://bugzilla.redhat.com/show_bug.cgi?id=1953286#c26 --- output/output_rhv.ml | 17 +++++++++++++++-- 1 file changed, 15 insertions(+), 2 deletions(-) diff --git a/output/output_rhv.ml b/output/output_rhv.ml index 5e3c031e38..f57ac9099e 100644 --- a/output/output_rhv.ml +++ b/output/output_rhv.ml @@ -175,8 +175,21 @@ module RHV = struct chmod filename 0o666 ) in - ignore (output_to_local_file ~changeuid - output_alloc output_format filename size socket) + let pid = output_to_local_file ~changeuid ~on_exit_kill:false + output_alloc output_format filename size socket in + + (* 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 + *) + 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) + ); ) (List.combine disks filenames); (* Save parameters since we need them during finalization. *) -- 2.37.0.rc2