[Libguestfs] [PATCH 06/18] rhv-upload: Fix cleanup after errors

[Daniel Erez]

On Mon, Nov 18, 2019 at 1:52 AM Nir Soffer <nsoffer at redhat.com> wrote:

> On Mon, Nov 18, 2019 at 1:04 AM Nir Soffer <nirsof at gmail.com> wrote:
> >
> > When request failed, we paused the transfer. This is not needed since
> > our intent it to cancel the transfer.
> >
> > When closing after failure, we canceled the transfer and removed the
> > disk. This is not needed since the transfer owns the disk and will
> > remove it when canceled.
> >
> > When finalizing times out, we canceled the transfer and removed the
> > disk. This is not needed since the transfer will clean it self, and
> > likely to fail because cancelling is not possible after finalizing.
>
> Daniel, can you confirm my assumptions about engine behaviour?
>
> > ---
> >  v2v/rhv-upload-plugin.py | 37 +++++++++++++++----------------------
> >  1 file changed, 15 insertions(+), 22 deletions(-)
> >
> > diff --git a/v2v/rhv-upload-plugin.py b/v2v/rhv-upload-plugin.py
> > index 5bdfdf49f..1f42c4a55 100644
> > --- a/v2v/rhv-upload-plugin.py
> > +++ b/v2v/rhv-upload-plugin.py
> > @@ -180,6 +180,10 @@ def open(readonly):
> >              inactivity_timeout = 3600,
> >          )
> >      )
> > +
> > +    # At this point the transfer owns the disk and will delete the disk
> if the
> > +    # transfer is canceled, or if finalizing the transfer fails.
> > +
> >      debug("transfer.id = %r" % transfer.id)
> >
> >      # Get a reference to the created transfer service.
> > @@ -309,15 +313,12 @@ def can_flush(h):
> >  def get_size(h):
> >      return params['disk_size']
> >
> > -# Any unexpected HTTP response status from the server will end up
> > -# calling this function which logs the full error, pauses the
> > -# transfer, sets the failed state, and raises a RuntimeError
> > -# exception.
> > +# Any unexpected HTTP response status from the server will end up
> calling this
> > +# function which logs the full error, sets the failed state, and raises
> a
> > +# RuntimeError exception.
> >  def request_failed(h, r, msg):
> > -    # Setting the failed flag in the handle causes the disk to be
> > -    # cleaned up on close.
> > +    # Setting the failed flag in the handle will cancel the transfer on
> close.
> >      h['failed'] = True
> > -    h['transfer_service'].pause()
> >
> >      status = r.status
> >      reason = r.reason
> > @@ -490,15 +491,10 @@ def flush(h):
> >
> >      r.read()
> >
> > -def delete_disk_on_failure(h):
> > -    transfer_service = h['transfer_service']
> > -    transfer_service.cancel()
> > -    disk_service = h['disk_service']
> > -    disk_service.remove()
> > -
> >  def close(h):
> >      http = h['http']
> >      connection = h['connection']
> > +    transfer_service = h['transfer_service']
> >
> >      # This is sometimes necessary because python doesn't set up
> >      # sys.stderr to be line buffered and so debug, errors or
> > @@ -508,15 +504,17 @@ def close(h):
> >
> >      http.close()
> >
> > -    # If the connection failed earlier ensure we clean up the disk.
> > +    # If the connection failed earlier ensure we cancel the trasfer.
> Canceling
> > +    # the transfer will delete the disk.
> >      if h['failed']:
> > -        delete_disk_on_failure(h)
>
> -        connection.close()
> > +        try:
> > +            transfer_service.cancel()
> > +        finally:
> > +            connection.close()
> >          return
> >
> >      try:
> >          disk = h['disk']
> > -        transfer_service = h['transfer_service']
> >
> >          transfer_service.finalize()
> >
> > @@ -548,11 +546,6 @@ def close(h):
> >          with builtins.open(params['diskid_file'], 'w') as fp:
> >              fp.write(disk.id)
> >
> > -    except:
> > -        # Otherwise on any failure we must clean up the disk.
> > -        delete_disk_on_failure(h)
>

still need to invoke transfer cancel for cleanup (to remove the disk)


> > -        raise
> > -
> >      finally:
> >          connection.close()
> >
> > --
> > 2.21.0
> >
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://listman.redhat.com/archives/libguestfs/attachments/20191118/7ba14dfa/attachment.htm>