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

[Oleg Vasilev]

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 at virtuozzo.com>
>> ---
>> 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
>>