[libvirt] [PATCH v2 0/2] qemu: fix misc noisy logs for optional storage sources
John, I still did not add anything to formatdomain.html.in because when I gave this a thought it looks odd to add something like "we don't get stats for missing storage source" - it is obvious because such getting stats is simply not possible. It you feel there is need to add something to the docs then please help me formulate it and we include it in the patch(es) on pushing. Nikolay Diff from v1: ============ - add error logs examples to commit messages - add notices at INFO level of handling optional disks with missing sources in a special way - comment why handling such cases in a special manner in code - check for cold_boot flag in qemuProcessPrepareHostStorage Nikolay Shirokovskiy (2): qemu: don't log error for missing optional storage sources on stats qemu: don't log error for missing optional storage sources on start src/qemu/qemu_domain.c | 9 +++++++++ src/qemu/qemu_domain.h | 3 +++ src/qemu/qemu_driver.c | 14 ++++++++++++++ src/qemu/qemu_process.c | 10 +++++++++- 4 files changed, 35 insertions(+), 1 deletion(-) -- 1.8.3.1
Every time we call all domain stats for inactive domain with unavailable storage source we get error message in logs [1]. It's a bit noisy. 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 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. 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 (version 1.2.15) which in turn fixes 596a13713 (version 1.2.12 )which added substantial stats for offline disks. [1] error message example: qemuOpenFileAs:3324 : Failed to open file '/path/to/optional/disk': No such file or directory Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com> --- src/qemu/qemu_domain.c | 9 +++++++++ src/qemu/qemu_domain.h | 3 +++ src/qemu/qemu_driver.c | 14 ++++++++++++++ 3 files changed, 26 insertions(+) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 1eb0e31..6f8df23 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -13696,3 +13696,12 @@ qemuDomainIsUsingNoShutdown(qemuDomainObjPrivatePtr priv) { return priv->monJSON && priv->allowReboot == VIR_TRISTATE_BOOL_YES; } + + +bool +qemuDomainDiskIsMissingLocalOptional(virDomainDiskDefPtr disk) +{ + return disk->startupPolicy == VIR_DOMAIN_STARTUP_POLICY_OPTIONAL && + virStorageSourceIsLocalStorage(disk->src) && disk->src->path && + !virFileExists(disk->src->path); +} diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h index 53b5ea1..b57fa19 100644 --- a/src/qemu/qemu_domain.h +++ b/src/qemu/qemu_domain.h @@ -1094,4 +1094,7 @@ qemuDomainRunningReasonToResumeEvent(virDomainRunningReason reason); bool qemuDomainIsUsingNoShutdown(qemuDomainObjPrivatePtr priv); +bool +qemuDomainDiskIsMissingLocalOptional(virDomainDiskDefPtr disk); + #endif /* __QEMU_DOMAIN_H__ */ diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index d0cf2c1..c2f3b74 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -20722,6 +20722,20 @@ qemuDomainGetStatsBlockExportDisk(virDomainDiskDefPtr disk, const char *backendstoragealias; int ret = -1; + /* + * This helps to keep logs clean from error messages on getting stats + * for optional disk with nonexistent source file. We won't get any + * stats for such a disk anyway in below code. + */ + if (!virDomainObjIsActive(dom) && + qemuDomainDiskIsMissingLocalOptional(disk)) { + VIR_INFO("optional disk '%s' source file is missing, " + "skip getting stats", disk->dst); + + return qemuDomainGetStatsBlockExportHeader(disk, disk->src, *recordnr, + records, nrecords); + } + for (n = disk->src; virStorageSourceIsBacking(n); n = n->backingStore) { if (blockdev) { frontendalias = QEMU_DOMAIN_DISK_PRIVATE(disk)->qomName; -- 1.8.3.1
On 12/13/18 6:02 AM, Nikolay Shirokovskiy wrote:
Every time we call all domain stats for inactive domain with unavailable storage source we get error message in logs [1]. It's a bit noisy. 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 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.
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 (version 1.2.15) which in turn fixes 596a13713 (version 1.2.12 )which added substantial stats for offline disks.
[1] error message example: qemuOpenFileAs:3324 : Failed to open file '/path/to/optional/disk': No such file or directory
Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com> --- src/qemu/qemu_domain.c | 9 +++++++++ src/qemu/qemu_domain.h | 3 +++ src/qemu/qemu_driver.c | 14 ++++++++++++++ 3 files changed, 26 insertions(+)
Reviewed-by: John Ferlan <jferlan@redhat.com> John
Because missing optional storage source is not error. The patch address only local files. Fixing other cases is a bit ugly. Below is example of error notice in log now: error: virStorageFileReportBrokenChain:427 : Cannot access storage file '/path/to/missing/optional/disk': No such file or directory Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com> --- src/qemu/qemu_process.c | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 2f8e19d..d754f09 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -6118,7 +6118,15 @@ qemuProcessPrepareHostStorage(virQEMUDriverPtr driver, if (!blockdev) virStorageSourceBackingStoreClear(disk->src); - if (qemuDomainDetermineDiskChain(driver, vm, disk, true) >= 0) + /* + * Go to applying startup policy for optional disk with nonexistent + * source file immediately as detemining chain will surely fail + * and we don't want noisy error notice in logs for this case. + */ + if (qemuDomainDiskIsMissingLocalOptional(disk) && cold_boot) + VIR_INFO("optional disk '%s' source file is missing, " + "skip checking disk chain", disk->dst); + else if (qemuDomainDetermineDiskChain(driver, vm, disk, true) >= 0) continue; if (qemuDomainCheckDiskStartupPolicy(driver, vm, idx, cold_boot) >= 0) -- 1.8.3.1
On 12/13/18 6:02 AM, Nikolay Shirokovskiy wrote:
Because missing optional storage source is not error. The patch address only local files. Fixing other cases is a bit ugly. Below is example of error notice in log now:
error: virStorageFileReportBrokenChain:427 : Cannot access storage file '/path/to/missing/optional/disk': No such file or directory
Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com> --- src/qemu/qemu_process.c | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-)
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 2f8e19d..d754f09 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -6118,7 +6118,15 @@ qemuProcessPrepareHostStorage(virQEMUDriverPtr driver, if (!blockdev) virStorageSourceBackingStoreClear(disk->src);
- if (qemuDomainDetermineDiskChain(driver, vm, disk, true) >= 0) + /* + * Go to applying startup policy for optional disk with nonexistent + * source file immediately as detemining chain will surely fail
determining
+ * and we don't want noisy error notice in logs for this case. + */ + if (qemuDomainDiskIsMissingLocalOptional(disk) && cold_boot) + VIR_INFO("optional disk '%s' source file is missing, " + "skip checking disk chain", disk->dst);
Perhaps the message here is "less important" (to me at least) than it was for the stats code, but it's also fine as is... with the spelling/typo fixed.. Reviewed-by: John Ferlan <jferlan@redhat.com> John
+ else if (qemuDomainDetermineDiskChain(driver, vm, disk, true) >= 0) continue;
if (qemuDomainCheckDiskStartupPolicy(driver, vm, idx, cold_boot) >= 0)
On 13.12.2018 14:02, Nikolay Shirokovskiy wrote:
John, I still did not add anything to formatdomain.html.in because when I gave this a thought it looks odd to add something like "we don't get stats for missing storage source" - it is obvious because such getting stats is simply not possible. It you feel there is need to add something to the docs then please help me formulate it and we include it in the patch(es) on pushing.
Nikolay
Diff from v1: ============
- add error logs examples to commit messages - add notices at INFO level of handling optional disks with missing sources in a special way - comment why handling such cases in a special manner in code - check for cold_boot flag in qemuProcessPrepareHostStorage
Nikolay Shirokovskiy (2): qemu: don't log error for missing optional storage sources on stats qemu: don't log error for missing optional storage sources on start
src/qemu/qemu_domain.c | 9 +++++++++ src/qemu/qemu_domain.h | 3 +++ src/qemu/qemu_driver.c | 14 ++++++++++++++ src/qemu/qemu_process.c | 10 +++++++++- 4 files changed, 35 insertions(+), 1 deletion(-)
Thanx for review, John! Pushed now. Nikolay
On 21.12.2018 12:22, Nikolay Shirokovskiy wrote:
On 13.12.2018 14:02, Nikolay Shirokovskiy wrote:
John, I still did not add anything to formatdomain.html.in because when I gave this a thought it looks odd to add something like "we don't get stats for missing storage source" - it is obvious because such getting stats is simply not possible. It you feel there is need to add something to the docs then please help me formulate it and we include it in the patch(es) on pushing.
Nikolay
Diff from v1: ============
- add error logs examples to commit messages - add notices at INFO level of handling optional disks with missing sources in a special way - comment why handling such cases in a special manner in code - check for cold_boot flag in qemuProcessPrepareHostStorage
Nikolay Shirokovskiy (2): qemu: don't log error for missing optional storage sources on stats qemu: don't log error for missing optional storage sources on start
src/qemu/qemu_domain.c | 9 +++++++++ src/qemu/qemu_domain.h | 3 +++ src/qemu/qemu_driver.c | 14 ++++++++++++++ src/qemu/qemu_process.c | 10 +++++++++- 4 files changed, 35 insertions(+), 1 deletion(-)
Thanx for review, John!
Pushed now.
Forget to add Reviewed-by:, again!
participants (2)
-
John Ferlan -
Nikolay Shirokovskiy