[libvirt] [PATCH v2 5/5] storage: Introduce 'managed' for the fchost parent

[John Ferlan]

On 11/10/2014 05:45 PM, John Ferlan wrote:
> https://bugzilla.redhat.com/show_bug.cgi?id=1160926
> 
> Introduce a 'managed' attribute to allow libvirt to decide whether to
> delete a vHBA vport created via external means such as nodedev-create.
> The code currently decides whether to delete the vHBA based solely on
> whether the parent was provided at creation time. However, that may not
> be the desired action, so rather than delete and force someone to create
> another vHBA via an additional nodedev-create allow the configuration of
> the storage pool to decide the desired action.
> 
> During createVport when libvirt does the VPORT_CREATE, set the managed
> value to YES if not already set to indicate to the deleteVport code that
> it should delete the vHBA when the pool is destroyed.
> 
> If libvirtd is restarted all the memory only state was lost, so for a
> persistent storage pool, use the virStoragePoolSaveConfig in order to
> write out the managed value.
> 
> Because we're now saving the current configuration, we need to be sure
> to not save the parent in the output XML if it was undefined at start.
> Saving the name would cause future starts to always use the same parent
> which is not the expected result when not providing a parent. By not
> providing a parent, libvirt is expected to find the best available
> vHBA port for each subsequent (re)start.
> 
> At deleteVport, use the new managed value to decide whether to execute
> the VPORT_DELETE.  Since we no longer save the parent in memory or in
> XML when provided, if it was not provided, then we have to look it up.
> 
> Signed-off-by: John Ferlan <jferlan at redhat.com>
> ---
>  docs/formatstorage.html.in                         | 13 +++
>  docs/schemas/basictypes.rng                        |  5 ++
>  src/conf/storage_conf.c                            | 17 ++++
>  src/conf/storage_conf.h                            |  1 +
>  src/storage/storage_backend_scsi.c                 | 93 +++++++++++++++++-----
>  .../pool-scsi-type-fc-host-managed.xml             | 15 ++++
>  .../pool-scsi-type-fc-host-managed.xml             | 18 +++++
>  tests/storagepoolxml2xmltest.c                     |  1 +
>  8 files changed, 143 insertions(+), 20 deletions(-)
>  create mode 100644 tests/storagepoolxml2xmlin/pool-scsi-type-fc-host-managed.xml
>  create mode 100644 tests/storagepoolxml2xmlout/pool-scsi-type-fc-host-managed.xml
> 

<...snip...>

Consider the following squished into deleteVport as testing asked about a
condition where someone does a 'virsh nodedev-destroy' on the vHBA
we've created resulting in the virGetFCHostNameByWWN() rightfully
returning NULL (just like it would in the createVport case when the
wwnn/wwpn doesn't exist).  Allowing virsh nodedev-destroy to remove an
"active" vHBA is perhaps a different issue...

The desire was to get a real error message instead of:

destroy the 'fc_host' pool
# virsh pool-destroy fc-pool
error: Failed to destroy pool fc-pool
error: An error occurred, but the cause is unknown

#

> diff --git a/src/storage/storage_backend_scsi.c b/src/storage/storage_backend_scsi.c
> index 41ea67a..b1602ea 100644
> --- a/src/storage/storage_backend_scsi.c
> +++ b/src/storage/storage_backend_scsi.c
<...snip...>
>  static int
> -deleteVport(virStoragePoolSourceAdapter adapter)
> +deleteVport(virConnectPtr conn,
> +            virStoragePoolSourceAdapter adapter)
>  {
>      unsigned int parent_host;
>      char *name = NULL;
> +    char *vhba_parent = NULL;
>      int ret = -1;
>  
>      if (adapter.type != VIR_STORAGE_POOL_SOURCE_ADAPTER_TYPE_FC_HOST)
>          return 0;
>  
> -    /* It must be a HBA instead of a vHBA as long as "parent"
> -     * is NULL. "createVport" guaranteed "parent" for a vHBA
> -     * cannot be NULL, it's either specified in XML, or detected
> -     * automatically.
> -     */
> -    if (!adapter.data.fchost.parent)
> +    VIR_DEBUG("conn=%p parent='%s', managed='%d' wwnn='%s' wwpn='%s'",
> +              conn, NULLSTR(adapter.data.fchost.parent),
> +              adapter.data.fchost.managed,
> +              adapter.data.fchost.wwnn,
> +              adapter.data.fchost.wwpn);
> +
> +    /* If we're not managing the deletion of the vHBA, then just return */
> +    if (adapter.data.fchost.managed != VIR_TRISTATE_BOOL_YES)
>          return 0;
>  
> +    /* Find our vHBA by searching the fc_host sysfs tree for our wwnn/wwpn */
>      if (!(name = virGetFCHostNameByWWN(NULL, adapter.data.fchost.wwnn,
>                                         adapter.data.fchost.wwpn)))
> -        return -1;
> -
> -    if (virGetSCSIHostNumber(adapter.data.fchost.parent, &parent_host) < 0)
>          goto cleanup;

diff --git a/src/storage/storage_backend_scsi.c b/src/storage/storage_backend_sc
index b1602ea..3f61610 100644
--- a/src/storage/storage_backend_scsi.c
+++ b/src/storage/storage_backend_scsi.c
@@ -764,8 +764,12 @@ deleteVport(virConnectPtr conn,
 
     /* Find our vHBA by searching the fc_host sysfs tree for our wwnn/wwpn */
     if (!(name = virGetFCHostNameByWWN(NULL, adapter.data.fchost.wwnn,
-                                       adapter.data.fchost.wwpn)))
+                                       adapter.data.fchost.wwpn))) {
+        virReportError(VIR_ERR_INTERNAL_ERROR,
+                       _("Failed to find fc_host for wwnn='%s' and wwpn='%s'"),
+                       adapter.data.fchost.wwnn, adapter.data.fchost.wwpn);
         goto cleanup;
+    }
 
     /* If at startup time we provided a parent, then use that to
      * get the parent_host value; otherwise, we have to determine


>  
> +    /* If at startup time we provided a parent, then use that to
> +     * get the parent_host value; otherwise, we have to determine
> +     * the parent scsi_host which we did not save at startup time
> +     */
> +    if (adapter.data.fchost.parent) {
> +        if (virGetSCSIHostNumber(adapter.data.fchost.parent, &parent_host) < 0)
> +            goto cleanup;
> +    } else {
> +        if (!(vhba_parent = getVhbaSCSIHostParent(conn, name)))
> +            goto cleanup;
> +
> +        if (virGetSCSIHostNumber(vhba_parent, &parent_host) < 0)
> +            goto cleanup;
> +    }
> +
>      if (virManageVport(parent_host, adapter.data.fchost.wwpn,
>                         adapter.data.fchost.wwnn, VPORT_DELETE) < 0)
>          goto cleanup;
> @@ -737,6 +789,7 @@ deleteVport(virStoragePoolSourceAdapter adapter)
>      ret = 0;
>   cleanup:
>      VIR_FREE(name);
> +    VIR_FREE(vhba_parent);
>      return ret;
>  }
>  

<...snip...>