[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH 1/2] storage: Alter check for default managed setting




On 07/18/2017 03:03 AM, Erik Skultety wrote:
> On Mon, Jul 03, 2017 at 02:52:05PM -0400, John Ferlan wrote:
>> Only alter the managed setting if it wasn't provided. If someone provided
>> 'no', then honor that rather than overwriting.
> 
> IMO this deserves to be tracked by a BZ, since the current behaviour is in
> contrast to what the documentation says.
> 
> ACK
> 
> Erik
> 

I think perhaps because it "bothered me" while testing the subsequent
patch, I looked at the logic again and thought, how is this right... The
myopia of the work didn't cause me to go backwards through history. So
if I go back to the bz that added the feature (commit id '5530f248'):

https://bugzilla.redhat.com/show_bug.cgi?id=1160926

The idea is/was to set 'managed' when this code created the vHBA and not
when it was already defined. IOW: If virVHBAGetHostByWWN returns @name,
then "something" already exists so we're not using any of the @parent*
fields (parent, parent_wwnn/wwpn, parent_fabric_name) in order to create
the vHBA, thus we should leave the managed as is and return.

Eventually, along came commit id '106930aaa' which altered the order of
the createVport code such that this code no longer could ascertain this
and the only way to "save" the vHBA is by using "no"; otherwise, we
forced usage of "managed='yes'".

So of course since this commit, we have way to know if we used an
existing vHBA (or HBA as it turns out) or if we created one. And we've
"essentially forced" the setting of "yes" which says delete the vHBA
when we're done.

Then as you point out in the reply to the next patch, commit id
'08c0ea16f' changes the return value from a 0,-1 to name or NULL. This
doesn't help us out at all in regards to whether we set managed and
creates even more problems (damn these refactors, they always seem to
mess something up ;-)).

So while I appreciate the ACK - I think I have to SNACK this one and
instead move the virVHBAGetHostByWWN check back into createVport. That
also includes moving the checkParent code back. This way, we'll have a
better handle on whether "something" exists. This would also fix the
checkParent check to follow what commit id '79ab0935' did

Fortunately (to a degree I suppose) "finding" an existing (real) vHBA
for a provided adapter wwnn/wwpn is "less" of a norm as the whole
purpose of all the parent* fields is because someone created a storage
pool and wanted it to manage creating the vHBA as opposed to someone
creating a vHBA themselves, then adding a storage pool for the sole
purpose to have the LUN's available via storage pool domain XML rather
than via direct access to the LUN's in non pool domain XML (if that
makes sense).

So while it "is" a bug, I'd say it's less likely (but not impossible)
that someone will actually be using such a configuration still. This is
a really, really long way to say I'm not sure I see value in creating a
bz honestly.

Thanks for taking me down memory lane ;-)

John

BTW: When all the above code was originally created, using the HBA for
the wwnn/wwpn of the <adapter> for the vhba wasn't even considered. It
was only someone created a vHBA via nodedev-create and that the
wwnn/wwpn was a vHBA. Hence the followup patch which will make that
check whether it was a vHBA or an HBA and cause failure based on that.

>>
>> Signed-off-by: John Ferlan <jferlan redhat com>
>> ---
>>  src/storage/storage_backend_scsi.c | 6 +++---
>>  1 file changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/src/storage/storage_backend_scsi.c b/src/storage/storage_backend_scsi.c
>> index f7378d3..f3e62fb 100644
>> --- a/src/storage/storage_backend_scsi.c
>> +++ b/src/storage/storage_backend_scsi.c
>> @@ -227,12 +227,12 @@ createVport(virConnectPtr conn,
>>                fchost->wwnn, fchost->wwpn);
>>
>>
>> -    /* Since we're creating the vHBA, then we need to manage removing it
>> +    /* Since we're creating the vHBA, then we may need to manage removing it
>>       * as well. Since we need this setting to "live" through a libvirtd
>>       * restart, we need to save the persistent configuration. So if not
>> -     * already defined as YES, then force the issue.
>> +     * already defined as YES or NO, then force the issue.
>>       */
>> -    if (fchost->managed != VIR_TRISTATE_BOOL_YES) {
>> +    if (fchost->managed == VIR_TRISTATE_BOOL_ABSENT) {
>>          fchost->managed = VIR_TRISTATE_BOOL_YES;
>>          if (configFile) {
>>              if (virStoragePoolSaveConfig(configFile, def) < 0)
>> --
>> 2.9.4
>>
>> --
>> libvir-list mailing list
>> libvir-list redhat com
>> https://www.redhat.com/mailman/listinfo/libvir-list


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]