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