[libvirt] [PATCH 1/2] qemu: don't log error for missing optional sources on stats

[John Ferlan]

$SUBJ:

'storage sources'

On 11/12/18 7:58 AM, Nikolay Shirokovskiy wrote:
> Every time we call all domain stats for inactive domain with
> unavailable source we get error message in logs. It's a bit noisy.

Are there ones in particular?

> While it's arguable whether we need such message or not for mandatory
> disks we would like not to see messages for optional disks. Let's

Filtering for the 'startupPolicy = optional' for domain stats (with
empty/unavailable local source) would seem to be a "new use" perhaps not
totally in line with the original intention.

> filter at least for cases of local files. Fixing other cases would
> require passing flag down the stack to .backendInit of storage
> which is ugly.

Yeah, I remember chasing down into backendInit (virStorageFileInitAs)
from qemuDomainStorageOpenStat for

   virStorageFileBackendForType:145 : internal error: missing storage
backend for 'none' storage
   virStorageFileBackendForType:141 : internal error: missing storage
backend for network files using iscsi protocol

where the 'none' comes from a volume using a storage pool of iSCSI
disks. I chased for a bit, but yes it got messy fast.

> 
> Stats for active domain are fine because we either drop disks
> with unavailable sources or clean source which is handled
> by virStorageSourceIsEmpty in qemuDomainGetStatsOneBlockFallback.
> 
> We have these logs for successful stats since 25aa7035d which
> in turn fixes 596a13713 which added substantial stats for offline
> disks.
> 
> Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy at virtuozzo.com>
> ---
>  src/qemu/qemu_driver.c  | 5 +++++
>  src/qemu/qemu_process.c | 9 +++++++++
>  src/qemu/qemu_process.h | 2 ++
>  3 files changed, 16 insertions(+)
> 
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index a52e249..72e4cfe 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -20290,6 +20290,11 @@ qemuDomainGetStatsBlockExportDisk(virDomainDiskDefPtr disk,
>      const char *backendstoragealias;
>      int ret = -1;
>  
> +    if (!virDomainObjIsActive(dom) &&
> +        qemuProcessMissingLocalOptionalDisk(disk))
> +        return qemuDomainGetStatsBlockExportHeader(disk, disk->src, *recordnr,
> +                                                   records, nrecords);
> +

>From my quick read this part would seem reasonable since the *Frontend
and *Backend wouldn't have valid data and *GetStatsOneBlock is fetching
source sizing data which for an empty source would be unobtainable.

>      for (n = disk->src; virStorageSourceIsBacking(n); n = n->backingStore) {
>          if (blockdev) {
>              frontendalias = QEMU_DOMAIN_DISK_PRIVATE(disk)->qomName;
> diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
> index 9cf9718..802274e 100644
> --- a/src/qemu/qemu_process.c
> +++ b/src/qemu/qemu_process.c
> @@ -6070,6 +6070,15 @@ qemuProcessPrepareSEVGuestInput(virDomainObjPtr vm)
>  }
>  
>  
> +bool
> +qemuProcessMissingLocalOptionalDisk(virDomainDiskDefPtr disk)

Curious why you chose qemu_process and not qemu_domain or even
virstoragefile?  This has nothing to do with whether the qemu process is
running or not.

> +{
> +    return disk->startupPolicy == VIR_DOMAIN_STARTUP_POLICY_OPTIONAL &&

While I agree in principle about optional disks; however, since
startupPolicy is optional it would seem this particular check is chasing
a very specific condition. Would it be reasonable to use a flag instead?
Something passed on the command line to essentially say to only collect
the name/format the name of the local empty sources.

That way we're not reusing something that has other uses. Although I'm
open to other opinions.

> +           virStorageSourceIsLocalStorage(disk->src) && disk->src->path &&

Use virStorageSourceIsEmpty instead.

> +           !virFileExists(disk->src->path);

And while I believe I understand why you chose this, it would seem to me
that it might be useful to know if an optional local disk had a path
disappear.  IOW: If all the other conditions are met, I'd like to know
that the source path is missing. That might be a good thing to know if
I'm getting stats for a domain. Maybe that's just me.

BTW: I fixed a bug in the domstats path recently where the "last error"
was lost (see commit e1fc7ec08).  Although I don't think that's what
you're chasing here.

John


> +}
> +
> +
>  static int
>  qemuProcessPrepareHostStorage(virQEMUDriverPtr driver,
>                                virDomainObjPtr vm,
> diff --git a/src/qemu/qemu_process.h b/src/qemu/qemu_process.h
> index 2037467..52d396d 100644
> --- a/src/qemu/qemu_process.h
> +++ b/src/qemu/qemu_process.h
> @@ -214,4 +214,6 @@ int qemuProcessStartManagedPRDaemon(virDomainObjPtr vm);
>  
>  void qemuProcessKillManagedPRDaemon(virDomainObjPtr vm);
>  
> +bool qemuProcessMissingLocalOptionalDisk(virDomainDiskDefPtr disk);
> +
>  #endif /* __QEMU_PROCESS_H__ */
>