[libvirt] [PATCH] rbd: Do not error out on a single image during pool refresh

[John Ferlan]

On 12/29/2015 08:35 AM, Wido den Hollander wrote:
> It could happen that rbd_list() returns X names, but that while
> refreshing the pool one of those RBD images is removed from Ceph
> through a different route then libvirt.
> 
> We do not need to error out in such case, we can simply ignore the
> volume and continue.
> 
>   error : volStorageBackendRBDRefreshVolInfo:289 :
>     failed to open the RBD image 'vol-998': No such file or directory
> 
> It could also be that one or more Placement Groups (PGs) inside Ceph
> are inactive due to a system failure.
> 
> If that happens it could be that some RBD images can not be refreshed
> and a timeout will be raised by librados.
> 
>   error : volStorageBackendRBDRefreshVolInfo:289 :
>     failed to open the RBD image 'vol-893': Connection timed out
> 
> Ignore the error and continue to refresh the rest of the pool's
> contents.
> 
> Signed-off-by: Wido den Hollander <wido at widodh.nl>
> ---
>  src/storage/storage_backend_rbd.c | 37 ++++++++++++++++++++++++++++---------
>  1 file changed, 28 insertions(+), 9 deletions(-)
> 

Seems reasonable - one nit...  This should be two patches..

> diff --git a/src/storage/storage_backend_rbd.c b/src/storage/storage_backend_rbd.c
> index 80684eb..888e3be 100644
> --- a/src/storage/storage_backend_rbd.c
> +++ b/src/storage/storage_backend_rbd.c
> @@ -279,18 +279,17 @@ static int volStorageBackendRBDRefreshVolInfo(virStorageVolDefPtr vol,
>                                                virStoragePoolObjPtr pool,
>                                                virStorageBackendRBDStatePtr ptr)
>  {
> -    int ret = -1;
> -    int r = 0;
> -    rbd_image_t image;
> +    int r = -1;
> +    rbd_image_t image = NULL;
> +    rbd_image_info_t info;
>  
>      r = rbd_open(ptr->ioctx, vol->name, &image, NULL);
>      if (r < 0) {
>          virReportSystemError(-r, _("failed to open the RBD image '%s'"),
>                               vol->name);
> -        return ret;
> +        goto cleanup;
>      }
>  
> -    rbd_image_info_t info;
>      r = rbd_stat(image, &info, sizeof(info));
>      if (r < 0) {
>          virReportSystemError(-r, _("failed to stat the RBD image '%s'"),
> @@ -308,6 +307,8 @@ static int volStorageBackendRBDRefreshVolInfo(virStorageVolDefPtr vol,
>      vol->type = VIR_STORAGE_VOL_NETWORK;
>      vol->target.format = VIR_STORAGE_FILE_RAW;
>  
> +    r = -1;
> +
>      VIR_FREE(vol->target.path);
>      if (virAsprintf(&vol->target.path, "%s/%s",
>                      pool->def->source.name,
> @@ -320,11 +321,13 @@ static int volStorageBackendRBDRefreshVolInfo(virStorageVolDefPtr vol,
>                      vol->name) == -1)
>          goto cleanup;
>  
> -    ret = 0;
> +    r = 0;
>  
>   cleanup:
> -    rbd_close(image);
> -    return ret;
> +    if (image)
> +        rbd_close(image);

Ahhh  a bug fix - should be a separate patch...

> +
> +    return r;

Changes in this function should have been their own patch since they fix
a bug.  Also, keep "ret = -1;' at the top and then use ret as the return
rather than adjusting 'r'.

John
>  }
>  
>  static int virStorageBackendRBDRefreshPool(virConnectPtr conn,
> @@ -399,7 +402,23 @@ static int virStorageBackendRBDRefreshPool(virConnectPtr conn,
>  
>          name += strlen(name) + 1;
>  
> -        if (volStorageBackendRBDRefreshVolInfo(vol, pool, &ptr) < 0) {
> +        r = volStorageBackendRBDRefreshVolInfo(vol, pool, &ptr);
> +
> +        /* It could be that a volume has been deleted through a different route
> +         * then libvirt and that will cause a -ENOENT to be returned.
> +         *
> +         * Another possibility is that there is something wrong with the placement
> +         * group (PG) that RBD image's header is in and that causes -ETIMEDOUT
> +         * to be returned.
> +         *
> +         * Do not error out and simply ignore the volume
> +         */
> +        if (r == -ENOENT || r == -ETIMEDOUT) {
> +            virStorageVolDefFree(vol);
> +            continue;
> +        }
> +
> +        if (r < 0) {
>              virStorageVolDefFree(vol);
>              goto cleanup;
>          }
>