6:58 a.m.
On Mon, Nov 18, 2019 at 1:52 AM Nir Soffer <nsoffer(a)redhat.com> wrote:
On Mon, Nov 18, 2019 at 1:04 AM Nir Soffer <nirsof(a)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
>