Re: [PATCH 2/2] logging: add log cleanup for obsolete domains

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 30.11.2022 16:49, Martin Kletzander wrote:
On Mon, Nov 21, 2022 at 03:29:57PM +0600, Oleg Vasilev wrote:
Before, logs from deleted machines have been piling up, since there were
no garbadge collection mechanism. Now virtlogd can be configured to
periodically scan log folder for orphan logs with no recent modfications
and delete it.

Signed-off-by: Oleg Vasilev <oleg.vasilev@xxxxxxxxxxxxx>
---
src/logging/log_daemon_config.c  |   9 +++
src/logging/log_daemon_config.h  |   3 +
src/logging/log_handler.c        | 108 +++++++++++++++++++++++++++++++
src/logging/test_virtlogd.aug.in |   2 +
src/logging/virtlogd.aug         |   2 +
src/logging/virtlogd.conf        |   7 ++
6 files changed, 131 insertions(+)

diff --git a/src/logging/log_daemon_config.c

...


+static void
+virLogHandlerMaybeCleanupObsoleteLog(virLogHandler *handler,
+                                     const char* path) {
+    size_t i;
+    bool remove = true;
+
+    if (fnmatch("*.log*", path, 0))
+        return;
+
+    virObjectLock(handler);

If you use a lock guard:

VIR_LOCK_GUARD lock = virObjectLockGuard(handler);

then ...
+    for (i = 0; i < handler->nfiles; i++) {
+        virLogHandlerLogFile *file = handler->files[i];
+        if (STRPREFIX(path, virRotatingFileWriterGetPath(file->file))) {
+            remove = false;
+            break;
+        }

... this condition body can just be replaced with `return`, and

+    }
+    virObjectUnlock(handler);
+
+    if (!remove)
+        return;
+

the unlocking and this above condition removed.

+    if (unlink(path) < 0) {
+        virReportSystemError(errno, _("Unable to delete %s"), path);
+    }
+}
+
+static void
+virLogHandlerCleanupObsoleteLogsFolder(virLogHandler *handler,
+                                       time_t oldest_to_keep,
+                                       const char *path,
+                                       int depth_left)
+{
+    DIR *dir;
+    struct dirent *entry;
+    char *newpath;
+    struct stat sb;
+
+    if (virDirOpenIfExists(&dir, path) < 0)
+        return;
+
+    while (virDirRead(dir, &entry, path) > 0) {
+        if (STREQ(entry->d_name, "."))
+            continue;
+        if (STREQ(entry->d_name, ".."))
+            continue;
+
+        newpath = g_strdup_printf("%s/%s", path, entry->d_name);
+

If you make this newpath a `g_autofree char *` in the while loop then
you can replace all `goto next` with `continue` and remove the label and
VIR_FREE() call.

Right, thanks, not yet used to the autoptr functionality in C.


+        if (stat(newpath, &sb) < 0) {
+            virReportSystemError(errno, _("Unable to stat %s"), newpath);
+            goto next;
+        }
+
+        if (S_ISDIR(sb.st_mode)) {
+            if (depth_left > 0)
+                virLogHandlerCleanupObsoleteLogsFolder(handler, oldest_to_keep, newpath, depth_left - 1);
+            goto next;
+        }
+
+        if (!S_ISREG(sb.st_mode)) {
+            goto next;
+        }
+
+        if (sb.st_mtim.tv_sec > oldest_to_keep) {
+            goto next;
+        }
+
+        virLogHandlerMaybeCleanupObsoleteLog(handler, newpath);
+
+ next:
+        VIR_FREE(newpath);
+    }
+
+    virDirClose(dir);
+}
+

...

diff --git a/src/logging/virtlogd.conf b/src/logging/virtlogd.conf
index c53a1112bd..d88ce31327 100644
--- a/src/logging/virtlogd.conf
+++ b/src/logging/virtlogd.conf
@@ -101,3 +101,10 @@
# Maximum number of backup files to keep. Defaults to 3,
# not including the primary active file
#max_backups = 3
+
+# Maximum age for log files to live after the last modification.
+# Defaults to 0, which means "forever".
+#max_age_days = 0
+
+# Root of all logs managed by virtlogd. Used to GC logs from obsolete machines.
+#log_root = "/var/log/libvirt"

One thing I am afraid of is that if there is some other log that is
unused for max_age_days, but is not managed by virlogd, then it will get
removed as well, but it might not be what users want.  Thankfully this
is an opt-in, but I would sleep more calmly if there was a warning with
explanation of what this might do outside of its scop


Yes, this is a valid concern. Warning here in config file would be helpful, maybe also warn on stdout during startup if this is enabled?

One more thing is that if larger logs are rotated, then this would
remove the old ones even if they are kept as number of backups.  That
seems like something that should be avoided (probably not even
configurable).

I think I will delete all log files (rotated and fresh) from one domain at the same moment, namely, when the fresh part becomes older than max_age_days.

Let me post a v2.

Thanks,
Oleg


--
2.38.1






[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]

  Powered by Linux