Re: [Libguestfs] [PATCH v2v v2] output: -o rhv-upload: Kill nbdkit instances before finalizing
On Wed, Feb 09, 2022 at 03:51:24PM +0200, Nir Soffer wrote:
On Wed, Feb 9, 2022 at 12:32 PM Richard W.M. Jones
<rjones(a)redhat.com> wrote:
> + (* 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;
Do we kill all nbdkits instances or only rhv-upload-plugin? For example
for vdsm output we try to access nbkit during finalization.
Only the ones created on the output side (which will be the ones using
rhv-upload-plugin.py), and only when using -o rhv-upload.
What if the process handles the SIGTERM and does not exit?
One example is a deadlock during termination.
It'll be a bug!
It will be more robust to wait for termination for a short time, and
repeat
the process with SIGKILL. Or just use SIGKILL to avoid the extra step.
If we handle removal of pid files and sockets, there is no need for clean
shutdown.
On the one hand it would mask the bug - I think if nbdkit + python +
rhv-upload-plugin.py doesn't shut down I want to find out about it (in
this case).
On the other hand it would probably continue and succeed OK because
all the data has been written by this point (otherwise nbdcopy --flush
shoudn't have returned).
Let's see if this causes problems.
> + 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" ^
> --
> 2.32.0
>
Otherwise the intent of terminating and waiting nbdkit before
finalizing sounds like the right way.
Rich.
--
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-df lists disk usage of guests without needing to install any
software inside the virtual machine. Supports Linux and Windows.
http://people.redhat.com/~rjones/virt-df/