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@xxxxxxxxxxxxx>
>> ---
>> 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__ */
>>
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
