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

[Nikolay Shirokovskiy]

On 11.12.2018 01:05, John Ferlan wrote:
> 
> $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?

Like this one:

qemuOpenFileAs:3324 : Failed to open file '/path/to/optinal/disk': No such file or directory

> 
>> 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.

But I was not going to change behaviour only to stop polluting
logs with messages like above.

> 
>> 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.

Yeah, we have nothing qemu specific here. Just not sure is this useful
for other purpuses 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.

Not sure I understand you. This patch does not change anything in respect
to collected stats so I can not see why we need extra flags.

> 
> 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.

But this is a bit different, say I use !virStorageSourceIsEmpty(src) instead of the line
above, then I can not sure disk->src->path even makes sense in the below check.

> 
>> +           !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.

Yeah, its arguable. But as getting stats is periodic with rather high
frequency seeing such log error over and over again in logs can be
tedious.

> 
> 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.

Nope, it's different case.

Nikolay

> 
> 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__ */
>>