[virt-manager PATCH 0/3] Create pool: show only available types

[Cole Robinson]

On 12/8/20 9:09 AM, Pino Toscano wrote:
> Hi Cole,
> 
> I'm reordering the paragraphs of your answer to ease mine.
> 
> On Monday, 7 December 2020 20:39:29 CET Cole Robinson wrote:
>> Can you explain your motivation a bit: Has this caused you issues
>> before?
> 
> My main motivation was to fix the fact that the pool creation dialog
> offers all the known types to virt-manager, even those that are not
> supported by the underlying connection. So I thought that, rather than
> letting the users select any of those unavailable ones to only get a
> "not supported" error afterwards, it seems better to offer only what
> can actually be created/setup.
> 

Yes I agree that it would be an improvement over the current UI to
report when we know a pool type is not going to work.

> Another thing came to my mind by reading this old Debian bug:
> https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=699701
> IIRC the "new volume" dialog used to offer a lot more formats in the
> past than only raw and qcow2. Still, that dialog has an editable field
> with the format, so you can change it to anything uncommon or even
> non-existing. So a possible idea was to validate that, both in the
> "new volume" dialog and when creating a new volume as result of
> virt-install --disk parameters, and warn on invalid formats.
> 

If libvirt throws an error message quickly enough I'm not really a fan
of duplicating validation in the app, unless there's a really good
reason. It's more code that needs testing. Worse is its an opportunity
for us to screw up and falsely reject something, so what started out as
a minor validation improvement can turn into a major blocking issue
(this has happened many times before).

Valid cases IMO are things like the new VM wizard where there are
multiple phases and we want to catch errors long before libvirt
interaction, or certain ambiguous situations where we want to point the
user in a better direction. There's other cases too but I don't think
volume formats apply.

Volume formats are problematic for other reasons too. Libvirt reports
'dmg' is supported but actually it's not available for volume creation
via qemu-img. reporting 'qcow' which basically no one wants could be
confusing for users looking for qcow2. RHEL compiles out support for
multiple formats but that isn't reflected in the capabilities list.
There's security concerns about some of the less common formats too IIRC

We used to have a hardcoded list of volume types in UI but I removed it
in favor of the current approach. I suspect 99% of users want qcow2 or
raw, and if they know they want something different then typing into the
field is not a big burden. That's my thinking

Using the storage capabilities to determine which storage pool types are
even _capable_ of creating file based volumes would replace a hardcoded
list we have in the code, so that's another net win, but still IMO not
worth the storagecapabilities call either.

>> Or is there something more useful in the storage XML that you
>> want to add support for afterwards?
> 
> One addition I wanted to do is supporting iscsi-direct pools, including
> their creation. Then I run into "hmm, that will make the combo in the
> new pool dialog even longer", so that got me into this series.
> 

That's fair.

Do you use iscsi-direct, or just notice it was missing? I've only heard
about Red Hat QE using iscsi-direct but it's fairly new in the scheme of
things so I could be missing something.

If you use it, do you use <auth> with it? Because there's presently no
auth UI, but I know the XML supports it.

>> The code looks fine but I am conflicted about this. I'm not sure it's
>> worth adding code to read and process storage capabilities XML at all.
>> I'd rather see the storage dialogs become smaller, not more
>> featureful/smarter at the cost of increased maintenance and potential
>> for future feature creep.
> 
> Considering what I wrote, I can see your POV, however on my defense
> I don't consider my suggestion that expensive (in terms of complexity
> & maintenance).
>

I agree, the code is straight forward. But it opens the door to more
code which I'm not sold on the net value of either.

Adding full coverage for this would raise the code amount too. For
example this API is not supported by libvirt's test driver. If we want
to test it in the uitest suite we will have to mock the API. We do that
already for capabilities and domcapabilities so there's an existing
pattern but it takes more work and consideration.

>> For example sheepdog and mpath can be dropped
>> entirely IMO. rbd pool creation should probably be dropped because the
>> UI is never going to be comprehensive enough to handle the common case
>> which requires specifying an auth secret. Same may apply to gluster but
>> I'd need to double check.
> 
> While I can agree with less common pool types like sheepdog and mpath,
> I'd consider ceph a nice thing to have, especially since it is not
> trivial. 

I hear you, but the 'not trivial' part is why I'm thinking it's not
really appropriate for the UI. IIRC you need to bend over backwards to
get ceph to work without any auth, and it's not a real world setup. So
most if not all storage pool users will need a secret. Right now as you
say virt-manager does not support secrets. So users already have to go
to the command line to get a ceph pool to work. Once they go that far
it's not much of a jump to ask them to write the pool XML too or find an
example online.

Yes better virt-manager UI could save them having to write XML
themselves which helps, but I don't think we can provide UI that's going
to trivialize setting up ceph. The only users who will be looking to set
up a pool in the first place will IMO already be advanced users who can
figure the rest out, or will be handed XML by a coworker or blog post
and be following verbatim steps.

TBH virt-manager really lacks the management of libvirt
> secrects, which would be immensely useful even for other kind of
> secrets. Worth creating an RFE ticket to at least not forget about it?
> 

I haven't seen any use of secrets compelling enough to me to add polling
to virtinst or the UI.

There are instances where the user needs to pass a secret UUID to the
XML in the virt-install case but that doesn't require any polling.
Storage pool secrets are not really compelling to me, see the above
comment about ceph and the DESIGN.md in general since I think all that
falls under 'advanced' usecases.

But I could be missing something, I know usage has grown in libvirt. I'm
interested in how you see it being used.

>> As implemented I'm a little iffy on the UI too. Just hiding options
>> without giving the user a hint can cause confusion, like why is FOO
>> available as a pool option for one connection but not the other. There's
>> ways to fix it but at the cost of more code with goes back to my point
>> above.
> 
> Hmm, I guess a possible way would be to make the available pool types
> as unselectable with a text like "zfs: Unavailable in this connection"?
> Similar to the URI combo in the "migrate vm" dialog.
> 

Yes that sounds reasonable.

Thanks,
Cole