libvirt: introduce VIR_DOMAIN_DESTROY_REMOVE_LOGS flag
The patch series based on discussion in RFC [1]. I wonder if we'd better add some property like "transient logs" instead of adding a flag to destoy API. Yes libguestfs uses virDomainDestroyFlags to terminate a VM and it is intended client of this new flag but it or other clients may want to use shutdown API or use autodestroy domains with same log behaviour. Then for the current task we only need to support this property in domain xml I guess. [1] removing VMs logs https://listman.redhat.com/archives/libvir-list/2022-February/msg00273.html Nikolay Shirokovskiy (3): libvirt: introduce VIR_DOMAIN_DESTROY_REMOVE_LOGS flag qemu: support VIR_DOMAIN_DESTROY_REMOVE_LOGS flag tools: support --remove-logs flag on destroing domain docs/manpages/virsh.rst | 7 +++++- include/libvirt/libvirt-domain.h | 1 + src/libvirt-domain.c | 6 +++++ src/qemu/qemu_domain.c | 41 ++++++++++++++++++++++++++++++++ src/qemu/qemu_domain.h | 4 ++++ src/qemu/qemu_driver.c | 8 ++++++- tools/virsh-domain.c | 8 ++++++- 7 files changed, 72 insertions(+), 3 deletions(-) -- 2.31.1
If this flag is set on calling virDomainDestroyFlags flags then remove per domain logs if possible. This can be used by libguestfs to delete logs for temporary domain. Otherwise such logs will stay wasting disk resources. Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com> --- include/libvirt/libvirt-domain.h | 1 + src/libvirt-domain.c | 6 ++++++ 2 files changed, 7 insertions(+) diff --git a/include/libvirt/libvirt-domain.h b/include/libvirt/libvirt-domain.h index 5f0a9b7572..b5356ddc8d 100644 --- a/include/libvirt/libvirt-domain.h +++ b/include/libvirt/libvirt-domain.h @@ -1236,6 +1236,7 @@ int virDomainDestroy (virDomainPtr domain); typedef enum { VIR_DOMAIN_DESTROY_DEFAULT = 0, /* Default behavior - could lead to data loss!! */ VIR_DOMAIN_DESTROY_GRACEFUL = 1 << 0, /* only SIGTERM, no SIGKILL */ + VIR_DOMAIN_DESTROY_REMOVE_LOGS = 1 << 1, /* remove VM logs on destroy */ } virDomainDestroyFlagsValues; int virDomainDestroyFlags (virDomainPtr domain, diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c index 5912551a49..0814d98e77 100644 --- a/src/libvirt-domain.c +++ b/src/libvirt-domain.c @@ -508,6 +508,12 @@ virDomainDestroy(virDomainPtr domain) * timeout; at that time, the management application can decide if * calling again without VIR_DOMAIN_DESTROY_GRACEFUL is appropriate. * + * If VIR_DOMAIN_DESTROY_REMOVE_LOGS flag is set then domain specific + * logs will be deleted as well if there are any. Note that not all + * deployments are be supported. For example in case of QEMU driver + * this flags is noop if virtlogd is not used for handling QEMU + * process output. + * * Another alternative which may produce cleaner results for the * guest's disks is to use virDomainShutdown() instead, but that * depends on guest support (some hypervisor/guest combinations may -- 2.31.1
Note that we attempt to remove logs only if virtlogd is in use. Otherwise we do not know the pattern for rotated files. For example for VM named "foo" we can not use "foo.log*" pattern to remove rotated logs as we can have VM named "foo.log" with log "foo.log.log". We can add extra check that filename does not end with ".log" but for VM "foo.log" we can have rotated log "foo.log.log.1". Ok let's check we don't have "log" in filename part corresponging to * but what if someone will use logrotate with "%Y.log-%m-%d" 'dateformat' option. In this case the check will exclude proper rotated files. Yes, the last example if quite artificial but it shows it is difficult to find out correctly rotated files when rotated files pattern is not known. Thus the above decision only to support case with virtlogd when we know the pattern. Another reason for not removing log files when logrotate is present is that due to races some files can escape deletion. For example foo.log.3 will be rotated to foo.log.4 after removing function will read directory files and thus foo.log.4 will not be deleted. Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com> --- src/qemu/qemu_domain.c | 41 +++++++++++++++++++++++++++++++++++++++++ src/qemu/qemu_domain.h | 4 ++++ src/qemu/qemu_driver.c | 8 +++++++- 3 files changed, 52 insertions(+), 1 deletion(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index a8401bac30..d1531bd77a 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -11592,3 +11592,44 @@ qemuDomainDeviceBackendChardevForeach(virDomainDef *def, DOMAIN_DEVICE_ITERATE_MISSING_INFO, &data); } + + +int +qemuDomainRemoveLogs(virQEMUDriver *driver, + const char *name) +{ + g_autoptr(virQEMUDriverConfig) cfg = NULL; + g_autofree char *format = NULL; + g_autofree char *main = NULL; + g_autoptr(DIR) dir = NULL; + struct dirent *entry; + int rc; + + cfg = virQEMUDriverGetConfig(driver); + if (!cfg->stdioLogD) + return 0; + + if (virDirOpen(&dir, cfg->logDir) < 0) + return -1; + + main = g_strdup_printf("%s.log", name); + format = g_strdup_printf("%s.log.%%u", name); + + while ((rc = virDirRead(dir, &entry, cfg->logDir)) > 0) { + unsigned int u; + + if (STREQ(entry->d_name, main) || + sscanf(entry->d_name, format, &u) == 1) { + g_autofree char *path = NULL; + + path = g_strdup_printf("%s/%s", cfg->logDir, entry->d_name); + if (unlink(path) && errno != ENOENT) + VIR_WARN("unlink(%s) error: %s", path, g_strerror(errno)); + } + } + + if (rc < 0) + return -1; + + return 0; +} diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h index e5046367e3..5a5d6daf71 100644 --- a/src/qemu/qemu_domain.h +++ b/src/qemu/qemu_domain.h @@ -1057,3 +1057,7 @@ int qemuDomainDeviceBackendChardevForeach(virDomainDef *def, qemuDomainDeviceBackendChardevForeachCallback cb, void *opaque); + +int +qemuDomainRemoveLogs(virQEMUDriver *driver, + const char *name); diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 65ac5ef367..c1cf30639a 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -2071,7 +2071,8 @@ qemuDomainDestroyFlags(virDomainPtr dom, int reason; bool starting; - virCheckFlags(VIR_DOMAIN_DESTROY_GRACEFUL, -1); + virCheckFlags(VIR_DOMAIN_DESTROY_GRACEFUL | + VIR_DOMAIN_DESTROY_REMOVE_LOGS, -1); if (!(vm = qemuDomainObjFromDomain(dom))) return -1; @@ -2111,6 +2112,11 @@ qemuDomainDestroyFlags(virDomainPtr dom, qemuProcessStop(driver, vm, VIR_DOMAIN_SHUTOFF_DESTROYED, QEMU_ASYNC_JOB_NONE, stopFlags); + + if ((flags & VIR_DOMAIN_DESTROY_REMOVE_LOGS) && + qemuDomainRemoveLogs(driver, vm->def->name) < 0) + VIR_WARN("Failed to remove logs for VM '%s'", vm->def->name); + event = virDomainEventLifecycleNewFromObj(vm, VIR_DOMAIN_EVENT_STOPPED, VIR_DOMAIN_EVENT_STOPPED_DESTROYED); -- 2.31.1
Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com> --- docs/manpages/virsh.rst | 7 ++++++- tools/virsh-domain.c | 8 +++++++- 2 files changed, 13 insertions(+), 2 deletions(-) diff --git a/docs/manpages/virsh.rst b/docs/manpages/virsh.rst index dd534c10cb..6946d033d5 100644 --- a/docs/manpages/virsh.rst +++ b/docs/manpages/virsh.rst @@ -1563,7 +1563,7 @@ destroy :: - destroy domain [--graceful] + destroy domain [--graceful] [--remove-logs] Immediately terminate the domain *domain*. This doesn't give the domain OS any chance to react, and it's the equivalent of ripping the power @@ -1582,6 +1582,11 @@ If *--graceful* is specified, don't resort to extreme measures (e.g. SIGKILL) when the guest doesn't stop after a reasonable timeout; return an error instead. +If *--remove-logs* is specified, remove per *domain* log files. Not all +deployment configuration can be supported. + +In case of QEMU the flag is only supported if virlogd is used to handle QEMU +process output. Otherwise the flag is ignored. domblkerror diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index b56f6a90f5..72e05b71a1 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -8220,6 +8220,10 @@ static const vshCmdOptDef opts_destroy[] = { .type = VSH_OT_BOOL, .help = N_("terminate gracefully") }, + {.name = "remove-logs", + .type = VSH_OT_BOOL, + .help = N_("remove domain logs") + }, {.name = NULL} }; @@ -8236,9 +8240,11 @@ cmdDestroy(vshControl *ctl, const vshCmd *cmd) if (vshCommandOptBool(cmd, "graceful")) flags |= VIR_DOMAIN_DESTROY_GRACEFUL; + if (vshCommandOptBool(cmd, "remove-logs")) + flags |= VIR_DOMAIN_DESTROY_REMOVE_LOGS; if (flags) - result = virDomainDestroyFlags(dom, VIR_DOMAIN_DESTROY_GRACEFUL); + result = virDomainDestroyFlags(dom, flags); else result = virDomainDestroy(dom); -- 2.31.1
Hi, all! What do you think? пн, 14 февр. 2022 г. в 15:19, Nikolay Shirokovskiy < nshirokovskiy@virtuozzo.com>:
The patch series based on discussion in RFC [1].
I wonder if we'd better add some property like "transient logs" instead of adding a flag to destoy API.
Yes libguestfs uses virDomainDestroyFlags to terminate a VM and it is intended client of this new flag but it or other clients may want to use shutdown API or use autodestroy domains with same log behaviour. Then for the current task we only need to support this property in domain xml I guess.
[1] removing VMs logs https://listman.redhat.com/archives/libvir-list/2022-February/msg00273.html
Nikolay Shirokovskiy (3): libvirt: introduce VIR_DOMAIN_DESTROY_REMOVE_LOGS flag qemu: support VIR_DOMAIN_DESTROY_REMOVE_LOGS flag tools: support --remove-logs flag on destroing domain
docs/manpages/virsh.rst | 7 +++++- include/libvirt/libvirt-domain.h | 1 + src/libvirt-domain.c | 6 +++++ src/qemu/qemu_domain.c | 41 ++++++++++++++++++++++++++++++++ src/qemu/qemu_domain.h | 4 ++++ src/qemu/qemu_driver.c | 8 ++++++- tools/virsh-domain.c | 8 ++++++- 7 files changed, 72 insertions(+), 3 deletions(-)
-- 2.31.1
пн, 28 февр. 2022 г. в 14:13, Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com>:
Hi, all!
What do you think?
Hi, please take a look at this series. Nikolay
пн, 14 февр. 2022 г. в 15:19, Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com>:
The patch series based on discussion in RFC [1].
I wonder if we'd better add some property like "transient logs" instead of adding a flag to destoy API.
Yes libguestfs uses virDomainDestroyFlags to terminate a VM and it is intended client of this new flag but it or other clients may want to use shutdown API or use autodestroy domains with same log behaviour. Then for the current task we only need to support this property in domain xml I guess.
[1] removing VMs logs https://listman.redhat.com/archives/libvir-list/2022-February/msg00273.html
Nikolay Shirokovskiy (3): libvirt: introduce VIR_DOMAIN_DESTROY_REMOVE_LOGS flag qemu: support VIR_DOMAIN_DESTROY_REMOVE_LOGS flag tools: support --remove-logs flag on destroing domain
docs/manpages/virsh.rst | 7 +++++- include/libvirt/libvirt-domain.h | 1 + src/libvirt-domain.c | 6 +++++ src/qemu/qemu_domain.c | 41 ++++++++++++++++++++++++++++++++ src/qemu/qemu_domain.h | 4 ++++ src/qemu/qemu_driver.c | 8 ++++++- tools/virsh-domain.c | 8 ++++++- 7 files changed, 72 insertions(+), 3 deletions(-)
-- 2.31.1
On 2/14/22 13:19, Nikolay Shirokovskiy wrote:
The patch series based on discussion in RFC [1].
I wonder if we'd better add some property like "transient logs" instead of adding a flag to destoy API.
Yes libguestfs uses virDomainDestroyFlags to terminate a VM and it is intended client of this new flag but it or other clients may want to use shutdown API or use autodestroy domains with same log behaviour. Then for the current task we only need to support this property in domain xml I guess.
[1] removing VMs logs https://listman.redhat.com/archives/libvir-list/2022-February/msg00273.html
Nikolay Shirokovskiy (3): libvirt: introduce VIR_DOMAIN_DESTROY_REMOVE_LOGS flag qemu: support VIR_DOMAIN_DESTROY_REMOVE_LOGS flag tools: support --remove-logs flag on destroing domain
docs/manpages/virsh.rst | 7 +++++- include/libvirt/libvirt-domain.h | 1 + src/libvirt-domain.c | 6 +++++ src/qemu/qemu_domain.c | 41 ++++++++++++++++++++++++++++++++ src/qemu/qemu_domain.h | 4 ++++ src/qemu/qemu_driver.c | 8 ++++++- tools/virsh-domain.c | 8 ++++++- 7 files changed, 72 insertions(+), 3 deletions(-)
Reviewed-by: Michal Privoznik <mprivozn@redhat.com> Sorry for delayed review. Michal
participants (3)
-
Michal Prívozník -
Nikolay Shirokovskiy -
Nikolay Shirokovskiy