$SUBJ:
'storage sources'
Post by Nikolay ShirokovskiyEvery 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?
Post by Nikolay ShirokovskiyWhile 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.
Post by Nikolay Shirokovskiyfilter 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.
Post by Nikolay ShirokovskiyStats 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.
---
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.
Post by Nikolay Shirokovskiyfor (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.
Post by Nikolay Shirokovskiy+{
+ 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.
Post by Nikolay Shirokovskiy+ virStorageSourceIsLocalStorage(disk->src) && disk->src->path &&
Use virStorageSourceIsEmpty instead.
Post by Nikolay Shirokovskiy+ !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
Post by Nikolay Shirokovskiy+}
+
+
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__ */