Bug 1792141

Sorry for the late answer.

The biggest issue is that the openstack output mode requires some output options (-oo), and currently the virt-p2v interface does not provide a way to specify them, as you also noticed. The rhv-upload output mode has the same issue, and because of that it is not available in the virt-p2v interface.
Hence, I submitted a patch to not show openstack as available output mode for now:
https://www.redhat.com/archives/libguestfs/2020-March/msg00127.html
A better fix for this will be to allow output options to be specified by the user; most probably it is about adding few UI knobs.

The second issue you notice is that there is no way to specify output options via kernel command line arguments. This is slightly more complicated though.

Hello

I'm using --cmdline to push parameter and OpenStack is not ignored in this way. I got also the same error "virt-v2v: error: openstack: -oo server-id=<NAME|UUID> not present"
Is it possible to add p2v.oo parameter in virt-p2v as you mentioned with a list of parameters defined by the user ? Even if it's only compatible with cmdline and kernel arguments ?
The responsibility to define valid parameters in p2v.oo section will be on the user side.

For cmdline, I guess we need to modify "generate-p2v-config.pl" file to add :
["p2v.output.openstack",    "p2v.oo"] in my @cmdline_aliases
and add
 "p2v.output.openstack" => manual_entry->new(
with the list -oo parameters like "server-id=xxx verify-server-certificate=false os-username=admin"


I'm using virt-p2v 1.42.2fedora release=1.1.el9 and virt-v2v 2.0.7rhel=9,release=7.el9

Thanks

Created attachment 1937412 [details]
Proposed Change to add -oo in cli and kernelcmd

Tested in cmdline and it works

(In reply to alecorps from comment #3)
> Created attachment 1937412 [details]
> Proposed Change to add -oo in cli and kernelcmd
> 
> Tested in cmdline and it works

Example of cmdline for the test:

/usr/bin/virt-p2v --cmdline='p2v.server=xxx p2v.username=root p2v.os=ssd p2v.identity=file:///var/tmp/id_rsa p2v.sudo=true p2v.guestname=xxx p2v.o=openstack p2v.oo="server-id=xxxx,os-auth-url=https://xxx.com:5000/v3,os-password=xxx,os-username=alecorps,os-project-name=xxx,os-region-name=xxx,os-project-domain-id=default,os-user-domain-name=Default"'

I'll investigate (I'm taking the BZ so I don't forget about it).

Created attachment 1937790 [details]
generate-p2v-config.pl

Created attachment 1937791 [details]
modification of conversion.c to add -oo

Please post patches rather than whole files, and also you can send them
(without subscribing) to libguestfs.  Ideally, use git-email.

A couple of thoughts on this bug in general:

virt-p2v tries to model virt-v2v output modes through the GUI and configuration.
This means that new output modes are difficult to add, especially if
they involve lots of custom configuration options, and -oo openstack
definitely falls into this category.

There's some case for allowing a free-form "extra virt-v2v parameters"
field.  This would let people use virt-p2v for a particular output
mode even before we have fully modelled the extra options in the GUI.
For some unusual options we might even leave it at that.

(1) The -oo options are output-specific. Therefore, mapping them to the
GUI is a problem.

The GUI currently side-steps this by adding helpful tooltips (with
gtk_widget_set_tooltip_markup()) to the -o, -oc, -os, -of entries. In
other words, the options we currently expose are already
output-specific, we just tell the user what option to ignore for what
output module.

This (= tooltips) can work the same with -oo, so consistency would not
be improved or deteriorated.

(2) The -oo option is extra messy. That's because multiple -oo options
can be set, and especially in case of the openstack output, even
virt-v2v uses a *passthrough* scheme, for handling -oo os-*=*. See
virt-v2v commits 54965103b2c7 ("v2v: Add -o openstack target, writes to
OpenStack & Cinder using APIs.", 2018-09-03) and 7e81c67eff03 ("v2v: -o
openstack: Restrict passthrough auth params to ‘os-*’.",
2018-11-20).

Now, from comment 6 and comment 7, I kind of "reconstructed" a unified
diff on top of virt-p2v commit f4c7ae6ba98f ("make-disk: inject EPEL9
repo in RHEL9 disk image", 2023-01-20). See it later, below. It seems
reasonable that the patch attempts to reuse the ConfigStringList
facility, for (effectively) turning a single text entry into a
comma-separated list of options. The problem (or, well, inconsistency)
with that is however that all the ConfigStringList knobs are currently
represented with actual list widgets on the GUI. Exposing a list (of
unspecified length!) of text entries would be a major complication for
the GUI, whereas exposing a single new text entry that would later be
split at commas would be inconsistent with the rest of the GUI.

(3) There's some historical inconsistency around output drivers. Commit
7a61a30f142c ("p2v: Add GUI controls for -o, -oc etc options on virt-v2v
command line.", 2014-09-02) introduced the text entries originally; it
open-coded a number of output modes, including open-coding "OpenStack
Glance" in the "-o" drop down list tooltip. Later, commit cff9e846693c
("p2v/v2v: List output drivers in `virt-v2v --machine-readable', and use
that in virt-p2v.", 2014-09-02) replaced the open-coded output module
list with dynamic fetching from virt-v2v, *but* the tooltip remained
statick. Yet later, commit b74c126629e3 ("Ignore 'openstack' driver",
2020-03-16) would filter "openstack" out of said dynamic fetching, but
we still have the "glance" reference in the tooltip. (I realize that
virt-v2v's "-o glance" and "-o openstack" differ; still, it remains a
confusing reference in virt-p2v.)

(4) The patch then, reconstructed:

> diff --git a/generate-p2v-config.pl b/generate-p2v-config.pl
> index 47487f7b0f35..0dc0f50491be 100755
> --- a/generate-p2v-config.pl
> +++ b/generate-p2v-config.pl
> @@ -147,6 +147,7 @@ my @fields = [
>        ConfigString->new(name => 'connection'),
>        ConfigString->new(name => 'format'),
>        ConfigString->new(name => 'storage'),
> +      ConfigStringList->new(name => 'openstack'),
>      ],
>    ),
>  ];

The naming here ("openstack") is too specific. According to
<https://libguestfs.org/virt-v2v.1.html#options>  as well, "-oo"
generally exists, only its interpretation is up to each output mode
("Set output option(s) related to the current output mode").

> @@ -168,6 +169,7 @@ my @cmdline_aliases = (
>    ["p2v.output.connection", "p2v.oc"],
>    ["p2v.output.format",     "p2v.of"],
>    ["p2v.output.storage",    "p2v.os"],
> +  ["p2v.output.openstack", "p2v.oo"],
>  );
>
>  # Some config entries are not exposed on the kernel command line.

This hunk should not be necessary; as the leading comment in the
(invisible) context says, "cmdline_aliases" is a compat layer. It was
introduced so that old (public) knobs would continue working after the
knobs were renamed. For newly introduced options, compatibility is not
required (there is no "old" public name to recognize).

> @@ -357,6 +359,12 @@ written to F</var/tmp>.",
>      description => "
>  Set the output allocation mode.  This is the same as the virt-v2v
>  I<-oa> option.  See L<virt-v2v(1)/OPTIONS>.",
> +  ),
> +  "p2v.output.openstack" => manual_entry->new(
> +    shortopt => "", # ignored for enums
> +    description => "
> +Set the output openstack.  This is the same as the virt-v2v
> +I<-oo> option.  See L<virt-v2v(1)/OPTIONS>.",
>    ),
>    "p2v.output.connection" => manual_entry->new(
>      shortopt => "URI",

Again, this language should be more generic. A hint at openstack could
be useful (which is an output module that heavily relies on -oo).

> diff --git a/conversion.c b/conversion.c
> index b9af47deda74..286a4fe84c80 100644
> --- a/conversion.c
> +++ b/conversion.c
> @@ -502,6 +502,14 @@ generate_wrapper_script (struct config *config, const char *remote_dir,
>      print_quoted (fp, config->output.type);
>    }
>
> +  if (config->output.openstack) {         /* -oo */
> +    int i;
> +    for (i = 0; config->output.openstack[i] != NULL; ++i) {
> +      fprintf (fp, " -oo ");
> +      print_quoted (fp, config->output.openstack[i]);
> +    }
> +  }
> +
>    switch (config->output.allocation) { /* -oa */
>    case OUTPUT_ALLOCATION_NONE:
>      /* nothing */

I think this hunk is nice, (again) relying on ConfigStringList.


IMO the main problem is how we represent such a list on the GUI whose
length is to be controlled by the user. That's a first -- it's
inherently incompatible with our current approach, namely that of
highlighting the "main" -o[...] options individually. With "-oo", the
utility of individually enumerating output options on the GUI mostly
vanishes.

And, in any case, the documentation (with the screenshots / diagrams)
would have to be updated.

(I've mostly only elaborated on what Rich already said in comment#8; in
a way I needed to explain it to myself.)

I can look into the "split at commas" idea later. It should probably be
done in start_conversion_clicked(), where we already "Unpack dialog
fields and check them"; the important thing is to split with the same
logic as the kernel cmdline parser does.

I believe I was wrong about the Glance comment being outdated, from commit 7a61a30f142c. Upstream virt-v2v supports glance [*], and -o glance doesn't seem to require (or even to handle) -oo. Therefore, the comment remains relevant.

[*] downstream doesn't, per bug 1977539; it's patched out of virt-v2v, and so virt-p2v doesn't have to filter out "glance" -- it's simply not there in the "--machine-readable" list of "output:..." lines from virt-v2v.

-o glance is quite a simple output mode.  I believe it requires
no(!) additional options, because it just runs the "glance" program
and all configuration is done using environment variables that
glance understands (or probably some config file).

It is indeed both discouraged upstream and removed in RHEL because
glance (templates / cattle) doesn't fit with the virt-v2v model (pets).

(In reply to Laszlo Ersek from comment #9)

> > @@ -168,6 +169,7 @@ my @cmdline_aliases = (
> >    ["p2v.output.connection", "p2v.oc"],
> >    ["p2v.output.format",     "p2v.of"],
> >    ["p2v.output.storage",    "p2v.os"],
> > +  ["p2v.output.openstack", "p2v.oo"],
> >  );
> >
> >  # Some config entries are not exposed on the kernel command line.
> 
> This hunk should not be necessary; as the leading comment in the
> (invisible) context says, "cmdline_aliases" is a compat layer. It was
> introduced so that old (public) knobs would continue working after the
> knobs were renamed. For newly introduced options, compatibility is not
> required (there is no "old" public name to recognize).

I'm revising this too -- while the compat knob is not required, the "p2v.oo" shorthand is simply more comfortable to write.

The splitting of ConfigStringList, from the kernel cmdline, is performed with guestfs_int_split_string() -- copied from libguestfs, into "libguestfs/guestfs-utils.c" --, in the generated file "kernel-config.c". This means two things:

- guestfs_int_split_string() should be usable when the knob is updated from the GUI (simple text entry),

- when generate_wrapper_script() is reached on the "headless" path, the resultant string list can indeed be NULL. guestfs_int_split_string() always creates at least one element in the output array (= the terminating NULL element), but in case the new param is not passed on the kernel cmdline, guestfs_int_split_string() is not called by the generated code at all, in my understanding, so the array itself can be NULL.

[p2v PATCH 00/11] Expose virt-v2v's "-oo"; re-enable openstack
Message-Id: <20230130142228.108135-1-lersek>
https://listman.redhat.com/archives/libguestfs/2023-January/030539.html

[p2v PATCH v2 00/11] Expose virt-v2v's "-oo"; re-enable openstack
Message-Id: <20230202102928.21597-1-lersek>
https://listman.redhat.com/archives/libguestfs/2023-February/030588.html

Upstream commit range f4c7ae6ba98f..62e92514e11f.

Because this BZ was filed for the upstream p2v project, I'm closing it as resolved now.