Re: [libvirt] [PATCH v2 1/3] storage: Fix existing parent check for vHBA creation

On Wed, Jul 19, 2017 at 09:59:06AM -0400, John Ferlan wrote: > > > On 07/19/2017 07:38 AM, Erik Skultety wrote: >> On Tue, Jul 18, 2017 at 11:10:38AM -0400, John Ferlan wrote: >>> https://bugzilla.redhat.com/show_bug.cgi?id=1472277 >>> >>> Commit id '106930aaa' altered the order of checking for an existing >>> vHBA (e.g something created via nodedev-create functionality outside >>> of the storage pool logic) which inadvertantly broke the code to >>> decide whether to alter/force the fchost->managed field to be 'yes' >>> because the storage pool will be managing the created vHBA in order >>> to ensure when the storage pool is destroyed that the vHBA is also >>> destroyed. >> >> Right, I agree with this - unless the user explicitly states they want the >> pre-created vHBA to be managed, we don't force any setting. I was wondering >> though, what about a use case when the user wants the vHBA to be auto-created, >> but non-managed at the same time...(yeah, I know I'm stretching it a bit with >> these unlikely scenarios, but then, you'd surely agree, you've already seen >> some of similar sort and one can never expect what creative ways of defining >> the XML are there to be found :)) > > I think you're becoming the new vHBA expert ;-) > > In this case, I tell them to go see figure one or go read the docs. From > the storage pool page, managed description: "For configurations that do > not provide an already created vHBA from a 'virsh nodedev-create', > libvirt will set this property to "yes"." > >> >> I was also about to point out in the previous version, that I didn't like how >> complex virNodeDeviceCreateVport was getting - do wwnn and wwpn relate to a >> vHBA at all or is it a regular HBA, does the vHBA exist already or should we >> actually create and manage it. >> > > The wwnn/wwpn are the wwnn/wwpn used to create the vHBA. Typically, a > SAN admin will "assign" the pair (there's some specific rules about > them). If not provided for a storage pool, then it's an XML error. For a > nodedev it's possible to have libvirt generate a wwnn/wwpn, which yes, > is quite confusing. In doing so libvirt uses a specific base and adds to > it (see virRandomGenerateWWN and cover at least one eye). > > I agree in general about the "complexity" thing, but as time has gone on > requests for new ways to determine which parent to use has caused code > bloat. Complexity is a time factored calculation. When originally > implemented using "host#" for parent was perfectly fine, but then > someone realized that it should be "scsi_host#". Then someone said, that > "scsi_host#" wasn't good enough because it can change between reboots. > So parent by wwnn/wwpn was added. During the discussions for that > someone else said what about using the fabric_wwn in order to find a > parent. Oh and the "future" holds being able to define multiple vHBA's > because it's felt migration of domains using vHBA pools is going to need > more than one vHBA because on the target host using the same wwnn/wwpn > won't work (although I have doubts here, but haven't had the cycles to > investigate). > > If you're interested, go read the tail end of the wiki > (http://wiki.libvirt.org/page/NPIV_in_libvirt) - it'll show a bit of the > history of how things looked at one time. > > TBH: Sometimes I think QE reads the code and figures out a way to create > bugs based on assumptions the code makes rather than making sure the > intended use cases "work properly". Hence this whole need to know > whether the parent is the HBA or the vHBA. Why would *anyone* provide > the HBA parent wwnn/wwpn when it's perfectly fine to create a storage > pool of the HBA without it via: > > <adapter type='scsi_host' name='scsi_host3'/> > > but no, someone wants to be inventive and think/believe: > > <adapter type='fc_host' [parent='scsi_host3'] wwnn='HBA_wwnn' > wwpn='HBA_wwpn'/> > > should work as well (where HBA_wwnn/wwpn are the wwnn/wwpn of scsi_host3 > in this example). > > The second XML isn't illegal, but because scsi_host3 has vHBA Well, I'd call it a misconfiguration, how can a device be a parent to itself? That's not what the attribute's supposed to serve and using it this way is a misuse - we should either ignore it in that case or return error. The storage pool won't start but it should never have in the first place and the XML def won't disappear in this case, so IMHO we could and probably should forbid it.

> characteristics (in this case the ability to create vports), thus it > passes certain tests and "could" be a different, but legal way to > essentially define the HBA as the storage pool. Wouldn't anyone use it > this way - probably not as there's no reason since the vHBA LUN's won't > be available. It's a digression + tirade ;-)... > >>> >>> This patch moves the check (and checkParent helper) for an existing >>> vHBA back into the createVport in storage_backend_scsi. It also >>> restores the logic back to what commit id '79ab0935' had with >> >> As I'm writing below, if you want to go for a partial revert, this patch needs >> to be split in 2, it's never a good idea to move code segments and doing >> adjustments to it at the same time. >> > > While I understand your point, I disagree. Purely moving the code > doesn't work as the code in node_device_conf returns @name and code in > storage_backend_scsi returns -1/0, so some amount of massaging the code > is going to be required anyway. May as well get it right in one shot > rather than confuse a future git bisector even more. Huh, you're right, let's go with your proposal then, with squashing in the tiny optimization snippet I've suggested in my previous response. Erik