6:31 a.m.
https://bugzilla.redhat.com/show_bug.cgi?id=1986386
My RHV instance is dead at the moment so I didn't do much more than
check this compiles and passes the one test we have. Also I want to
spend as little time as possible on RHV outputs for virt-v2v since the
RHV product will be discontinued soon.
I did want to point out some things:
- The preceeding code is probably wrong.
https://github.com/libguestfs/virt-v2v/blob/master/output/rhv-upload-tran...
It attempts to search for the output storage using:
storage_domains = system_service.storage_domains_service().list(
search='name=%s' % params['output_storage'],
case_sensitive=True,
)
I couldn't find any documentation about what can go into that
search string, but it's clearly a lot more complicated than just
pasting in the literal name after "name=". At the very least,
spaces are not permitted, see:
https://github.com/libguestfs/virt-v2v/blob/master/output/rhv-upload-tran...
- The bug reporter used "data*" as the name and I suspect that is
parsed in some way (wildcard? regexp? I've no idea).
- Probably for the same reason, the preceeding code ought to fail
with an error if the output storage domain doesn't exist. The fact
we reach the code patched here at all also indicates some bug,
maybe in the search string.
As I say above, I don't especially care about any of this.
Rich.
6:31 a.m.
New subject: [PATCH v2v] -o rhv-upload: Give a nicer error if the storage domain does not exist
Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1986386
Reported-by: Junqin Zhou
---
output/rhv-upload-precheck.py | 7 ++++++-
1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/output/rhv-upload-precheck.py b/output/rhv-upload-precheck.py
index 1dc1b8498a..35ea021032 100644
--- a/output/rhv-upload-precheck.py
+++ b/output/rhv-upload-precheck.py
@@ -81,7 +81,12 @@ datacenter = data_centers[0]
# Get the storage domain.
storage_domains = connection.follow_link(datacenter.storage_domains)
-storage_domain = [sd for sd in storage_domains if sd.name ==
params['output_storage']][0]
+try:
+ storage_domain = [sd for sd in storage_domains \
+ if sd.name == params['output_storage']][0]
+except IndexError:
+ raise RuntimeError("The storage domain ‘%s’ does not exist" %
+ params['output_storage'])
# Get the cluster.
clusters = connection.follow_link(datacenter.clusters)
--
2.39.0
5:18 a.m.
New subject: [PATCH v2v] -o rhv-upload: Give a nicer error if the storage domain does not exist
On Thu, Jan 26, 2023 at 2:31 PM Richard W.M. Jones <rjones(a)redhat.com> wrote:
Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1986386
Reported-by: Junqin Zhou
---
output/rhv-upload-precheck.py | 7 ++++++-
1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/output/rhv-upload-precheck.py b/output/rhv-upload-precheck.py
index 1dc1b8498a..35ea021032 100644
--- a/output/rhv-upload-precheck.py
+++ b/output/rhv-upload-precheck.py
@@ -81,7 +81,12 @@ datacenter = data_centers[0]
# Get the storage domain.
storage_domains = connection.follow_link(datacenter.storage_domains)
-storage_domain = [sd for sd in storage_domains if sd.name ==
params['output_storage']][0]
+try:
+ storage_domain = [sd for sd in storage_domains \
+ if sd.name == params['output_storage']][0]
Using `\` may work but it is needed. You can do this:
storage_domain = [sd for sd in storage_domains
if sd.name == params['output_storage']][0]
This is also the common way to indent list comprehension that
makes the expression more clear.
+except IndexError:
+ raise RuntimeError("The storage domain ‘%s’ does not exist" %
+ params['output_storage'])
The fix is safe and makes sense.
Not sure why we list all storage domains when we already know the name,
maybe Albert would like to clean up this mess later.
Nir
5:32 a.m.
New subject: [PATCH v2v] -o rhv-upload: Give a nicer error if the storage domain does not exist
On Fri, Jan 27, 2023 at 1:18 PM Nir Soffer <nsoffer(a)redhat.com> wrote:
On Thu, Jan 26, 2023 at 2:31 PM Richard W.M. Jones <rjones(a)redhat.com> wrote:
>
> Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1986386
> Reported-by: Junqin Zhou
> ---
> output/rhv-upload-precheck.py | 7 ++++++-
> 1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/output/rhv-upload-precheck.py b/output/rhv-upload-precheck.py
> index 1dc1b8498a..35ea021032 100644
> --- a/output/rhv-upload-precheck.py
> +++ b/output/rhv-upload-precheck.py
> @@ -81,7 +81,12 @@ datacenter = data_centers[0]
>
> # Get the storage domain.
> storage_domains = connection.follow_link(datacenter.storage_domains)
> -storage_domain = [sd for sd in storage_domains if sd.name ==
params['output_storage']][0]
> +try:
> + storage_domain = [sd for sd in storage_domains \
> + if sd.name == params['output_storage']][0]
Using `\` may work but it is needed. You can do this:
storage_domain = [sd for sd in storage_domains
if sd.name == params['output_storage']][0]
This is also the common way to indent list comprehension that
makes the expression more clear.
> +except IndexError:
> + raise RuntimeError("The storage domain ‘%s’ does not exist" %
> + params['output_storage'])
The fix is safe and makes sense.
Not sure why we list all storage domains when we already know the name,
maybe Albert would like to clean up this mess later.
Like this:
https://github.com/oVirt/python-ovirt-engine-sdk4/blob/2aa50266056b7ee0b7...
5:11 a.m.
On Thu, Jan 26, 2023 at 2:31 PM Richard W.M. Jones <rjones(a)redhat.com> wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=1986386
My RHV instance is dead at the moment so I didn't do much more than
check this compiles and passes the one test we have. Also I want to
spend as little time as possible on RHV outputs for virt-v2v since the
RHV product will be discontinued soon.
I did want to point out some things:
- The preceeding code is probably wrong.
https://github.com/libguestfs/virt-v2v/blob/master/output/rhv-upload-tran...
It attempts to search for the output storage using:
storage_domains = system_service.storage_domains_service().list(
search='name=%s' % params['output_storage'],
case_sensitive=True,
)
I think the search is correct. This is explained in
https://bugzilla.redhat.com/1986386#c1
I couldn't find any documentation about what can go into that
search string, but it's clearly a lot more complicated than just
pasting in the literal name after "name=". At the very least,
spaces are not permitted, see:
https://github.com/libguestfs/virt-v2v/blob/master/output/rhv-upload-tran...
True, search can be an expression.
- The bug reporter used "data*" as the name and I suspect
that is
parsed in some way (wildcard? regexp? I've no idea).
It is treated as glob pattern, also explained in comment 1.
- Probably for the same reason, the preceeding code ought to fail
with an error if the output storage domain doesn't exist. The fact
we reach the code patched here at all also indicates some bug,
maybe in the search string.
As I say above, I don't especially care about any of this.
I'm not working on RHV since August 2022. Adding Albert who is current
RHV storage maintainer.
Nir
698
days inactive
699
days old
4 comments
2 participants
participants (2)
-
Nir Soffer -
Richard W.M. Jones