[libvirt PATCH] qemu: Shorten domain name in virtiofsd log path
This helps when starting domains with long names which could possibly end up creating too long of a name for the filesystem. The path is not being saved in the domain config, so there is no need for backwards compatibility. User aliases will always start with "ua-" and the only way paths could collide is to: 1) create a domain named e.g. "asdf-ua" 2) start the domain with virtiofsd debug logs 3) destroy the domain 4) restart libvirt daemon for domain IDs to start from 1 again 5) create a domain named "asdf" 6) add user alias for the virtiofs that is the same as was generated for the first domain, with the required "ua-" prefix 7) start the "asdf" domain at which point the logs for the two domains would end up in the same logfile. Since this is still better than what we had before I think it is not worth fixing this peculiar scenario in this patch. Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1817401 Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- src/qemu/qemu_virtiofs.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/qemu/qemu_virtiofs.c b/src/qemu/qemu_virtiofs.c index 7e3324b017ce..67df8b3890b6 100644 --- a/src/qemu/qemu_virtiofs.c +++ b/src/qemu/qemu_virtiofs.c @@ -81,8 +81,9 @@ qemuVirtioFSCreateLogFilename(virQEMUDriverConfig *cfg, const char *alias) { g_autofree char *name = NULL; + g_autofree char *shortname = virDomainDefGetShortName(def); - name = g_strdup_printf("%s-%s", def->name, alias); + name = g_strdup_printf("%s-%s", shortname, alias); return virFileBuildPath(cfg->logDir, name, "-virtiofsd.log"); } -- 2.35.1
On a Thursday in 2022, Martin Kletzander wrote:
This helps when starting domains with long names which could possibly end up creating too long of a name for the filesystem.
The path is not being saved in the domain config, so there is no need for backwards compatibility. User aliases will always start with "ua-" and the only way paths could collide is to:
1) create a domain named e.g. "asdf-ua" 2) start the domain with virtiofsd debug logs 3) destroy the domain 4) restart libvirt daemon for domain IDs to start from 1 again 5) create a domain named "asdf" 6) add user alias for the virtiofs that is the same as was generated for the first domain, with the required "ua-" prefix 7) start the "asdf" domain
at which point the logs for the two domains would end up in the same logfile.
Since this is still better than what we had before I think it is not worth fixing this peculiar scenario in this patch.
I don't think this is better. It does fix it for the strange use case of using very long domain names, and I agree that people combining domain names with "-ua" suffixes and no user aliases along with exact same domain names without the -ua suffix, but using user aliases are not worth worrying about. But for the 99 % of reasonable users, this patch includes the domain ID in the log path, so the path can change across restarts of the same domain. I don't like including this transient identifier in the log path. We also don't shorten the domain log, since we expect it to stay after the domain stops running. Jano
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1817401 Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- src/qemu/qemu_virtiofs.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
On Thu, Apr 07, 2022 at 02:14:30PM +0200, Ján Tomko wrote:
On a Thursday in 2022, Martin Kletzander wrote:
This helps when starting domains with long names which could possibly end up creating too long of a name for the filesystem.
The path is not being saved in the domain config, so there is no need for backwards compatibility. User aliases will always start with "ua-" and the only way paths could collide is to:
1) create a domain named e.g. "asdf-ua" 2) start the domain with virtiofsd debug logs 3) destroy the domain 4) restart libvirt daemon for domain IDs to start from 1 again 5) create a domain named "asdf" 6) add user alias for the virtiofs that is the same as was generated for the first domain, with the required "ua-" prefix 7) start the "asdf" domain
at which point the logs for the two domains would end up in the same logfile.
Since this is still better than what we had before I think it is not worth fixing this peculiar scenario in this patch.
I don't think this is better. It does fix it for the strange use case of using very long domain names, and I agree that people combining domain names with "-ua" suffixes and no user aliases along with exact same domain names without the -ua suffix, but using user aliases are not worth worrying about.
But for the 99 % of reasonable users, this patch includes the domain ID in the log path, so the path can change across restarts of the same domain. I don't like including this transient identifier in the log path. We also don't shorten the domain log, since we expect it to stay after the domain stops running.
I think it is even better. If you are looking at the virtiofsd debug log then you probably want to look at the current run, you can see when the file was created and open the newest one. And if you need to look at all the log files, then you can just: cat $(ls -cr *-$domain-$alias-virtiofsd.log) | less or similar. But maybe my assumption about virtiofsd debug logs being usable only in corner case scenarios is wrong. I wouldn't hate domain logfiles including the id either.
Jano
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1817401 Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- src/qemu/qemu_virtiofs.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
On Thu, Apr 07, 2022 at 03:19:27PM +0200, Martin Kletzander wrote:
On Thu, Apr 07, 2022 at 02:14:30PM +0200, Ján Tomko wrote:
On a Thursday in 2022, Martin Kletzander wrote:
This helps when starting domains with long names which could possibly end up creating too long of a name for the filesystem.
The path is not being saved in the domain config, so there is no need for backwards compatibility. User aliases will always start with "ua-" and the only way paths could collide is to:
1) create a domain named e.g. "asdf-ua" 2) start the domain with virtiofsd debug logs 3) destroy the domain 4) restart libvirt daemon for domain IDs to start from 1 again 5) create a domain named "asdf" 6) add user alias for the virtiofs that is the same as was generated for the first domain, with the required "ua-" prefix 7) start the "asdf" domain
at which point the logs for the two domains would end up in the same logfile.
Since this is still better than what we had before I think it is not worth fixing this peculiar scenario in this patch.
I don't think this is better. It does fix it for the strange use case of using very long domain names, and I agree that people combining domain names with "-ua" suffixes and no user aliases along with exact same domain names without the -ua suffix, but using user aliases are not worth worrying about.
But for the 99 % of reasonable users, this patch includes the domain ID in the log path, so the path can change across restarts of the same domain. I don't like including this transient identifier in the log path. We also don't shorten the domain log, since we expect it to stay after the domain stops running.
I think it is even better. If you are looking at the virtiofsd debug log then you probably want to look at the current run, you can see when the file was created and open the newest one. And if you need to look at all the log files, then you can just:
cat $(ls -cr *-$domain-$alias-virtiofsd.log) | less
or similar. But maybe my assumption about virtiofsd debug logs being usable only in corner case scenarios is wrong.
I wouldn't hate domain logfiles including the id either.
That would lead to a big growth in the number of log files, as we would be creating a new one every time, instead of appending to the existing one. With regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
participants (3)
-
Daniel P. Berrangé -
Ján Tomko -
Martin Kletzander