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

[Richard W.M. Jones]

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 at 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/