Libguestfs - Feb 2020 - Re: [PATCH v2v v2 2/2] rhv-upload: Check that rhv-disk-uuid is not already taken (RHBZ#1789279)

On Wednesday, 29 January 2020 15:34:49 CET Martin Kletzander
wrote:
> This makes the code fail with a sensible error message instead of cryptic
error
> from ovirtsdk.
> 
> Signed-off-by: Martin Kletzander <mkletzan@redhat.com>
> ---
>  v2v/rhv-upload-plugin.py | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/v2v/rhv-upload-plugin.py b/v2v/rhv-upload-plugin.py
> index d3e6260e97f4..413ad53b05ab 100644
> --- a/v2v/rhv-upload-plugin.py
> +++ b/v2v/rhv-upload-plugin.py
> @@ -26,6 +26,9 @@ import ssl
>  import sys
>  import time
>  
> +import nbdkit
> +import errno
> +
>  from http.client import HTTPSConnection, HTTPConnection
>  from urllib.parse import urlparse
>  
> @@ -461,6 +464,15 @@ def create_disk(connection):
>      system_service = connection.system_service()
>      disks_service = system_service.disks_service()
>  
> +    uuid = params.get('rhv_disk_uuid')
> +    if uuid is not None:
> +        try:
> +            disks_service.disk_service(uuid).get()
> +            nbdkit.set_error(errno.EEXIST)
> +            raise ValueError("Disk with the UUID '%s' already
exists" % uuid)
> +        except sdk.NotFoundError:
> +            pass
This check seems correct to me, although it is done too late: IMHO this
is something to do in the precheck script, so we do not even attempt to
connect to the source if any of the specified UUIDs already exists in
oVirt.

-- 
Pino Toscano

On Mon, Feb 24, 2020 at 10:28:46AM +0100, Pino Toscano
wrote:
>On Wednesday, 29 January 2020 15:34:49 CET Martin Kletzander wrote:
>> This makes the code fail with a sensible error message instead of
cryptic error
>> from ovirtsdk.
>>
>> Signed-off-by: Martin Kletzander <mkletzan@redhat.com>
>> ---
>>  v2v/rhv-upload-plugin.py | 12 ++++++++++++
>>  1 file changed, 12 insertions(+)
>>
>> diff --git a/v2v/rhv-upload-plugin.py b/v2v/rhv-upload-plugin.py
>> index d3e6260e97f4..413ad53b05ab 100644
>> --- a/v2v/rhv-upload-plugin.py
>> +++ b/v2v/rhv-upload-plugin.py
>> @@ -26,6 +26,9 @@ import ssl
>>  import sys
>>  import time
>>
>> +import nbdkit
>> +import errno
>> +
>>  from http.client import HTTPSConnection, HTTPConnection
>>  from urllib.parse import urlparse
>>
>> @@ -461,6 +464,15 @@ def create_disk(connection):
>>      system_service = connection.system_service()
>>      disks_service = system_service.disks_service()
>>
>> +    uuid = params.get('rhv_disk_uuid')
>> +    if uuid is not None:
>> +        try:
>> +            disks_service.disk_service(uuid).get()
>> +            nbdkit.set_error(errno.EEXIST)
>> +            raise ValueError("Disk with the UUID '%s'
already exists" % uuid)
>> +        except sdk.NotFoundError:
>> +            pass
>
>This check seems correct to me, although it is done too late: IMHO this
>is something to do in the precheck script, so we do not even attempt to
>connect to the source if any of the specified UUIDs already exists in
>oVirt.
>
Well at that point we are not passing the UUIDs to the script and that's why
I
selected this one (if I remember correctly).  Given how often this would happen
I don't think it would be that big of a deal to reject it slightly slower. 
Also
the race window (of the UUID being created in the meantime) would become bigger.
>--
>Pino Toscano

On Monday, 24 February 2020 12:05:55 CET Martin Kletzander
wrote:
> On Mon, Feb 24, 2020 at 10:28:46AM +0100, Pino Toscano wrote:
> >On Wednesday, 29 January 2020 15:34:49 CET Martin Kletzander wrote:
> >> This makes the code fail with a sensible error message instead of
cryptic error
> >> from ovirtsdk.
> >>
> >> Signed-off-by: Martin Kletzander <mkletzan@redhat.com>
> >> ---
> >>  v2v/rhv-upload-plugin.py | 12 ++++++++++++
> >>  1 file changed, 12 insertions(+)
> >>
> >> diff --git a/v2v/rhv-upload-plugin.py b/v2v/rhv-upload-plugin.py
> >> index d3e6260e97f4..413ad53b05ab 100644
> >> --- a/v2v/rhv-upload-plugin.py
> >> +++ b/v2v/rhv-upload-plugin.py
> >> @@ -26,6 +26,9 @@ import ssl
> >>  import sys
> >>  import time
> >>
> >> +import nbdkit
> >> +import errno
> >> +
> >>  from http.client import HTTPSConnection, HTTPConnection
> >>  from urllib.parse import urlparse
> >>
> >> @@ -461,6 +464,15 @@ def create_disk(connection):
> >>      system_service = connection.system_service()
> >>      disks_service = system_service.disks_service()
> >>
> >> +    uuid = params.get('rhv_disk_uuid')
> >> +    if uuid is not None:
> >> +        try:
> >> +            disks_service.disk_service(uuid).get()
> >> +            nbdkit.set_error(errno.EEXIST)
> >> +            raise ValueError("Disk with the UUID
'%s' already exists" % uuid)
> >> +        except sdk.NotFoundError:
> >> +            pass
> >
> >This check seems correct to me, although it is done too late: IMHO this
> >is something to do in the precheck script, so we do not even attempt to
> >connect to the source if any of the specified UUIDs already exists in
> >oVirt.
> >
> 
> Well at that point we are not passing the UUIDs to the script and
that's why I
> selected this one (if I remember correctly).
We can always add new parameters to the scripts - they take a JSON with
the parameters already.
> Given how often this would happen I don't think it would be that big
> of a deal to reject it slightly slower.
My thought is that, since we already have these parameters, checking
them earlier avoids issues later on.
> Also the race window (of the UUID being created in the meantime) would
> become bigger.
Yes, this is a possibility, albeit IMHO very remote (and it will error
out anyway, even if with the current semi-criptic message).

-- 
Pino Toscano