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