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

[Nir Soffer]

On Mon, Nov 18, 2019 at 2:58 PM Daniel Erez <derez at redhat.com> wrote:
>
>
>
> 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)

Good point. If engine fail to finalize, it can move to "paused by
system" state, and will
not delete the disk.

Will fix in v2.

>
>>
>> > -        raise
>> > -
>> >      finally:
>> >          connection.close()
>> >
>> > --
>> > 2.21.0
>> >
>>