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