[libvirt PATCH 5/6] Make PATHs unique for a VM object instance

Shaju Abraham shaju.abraham at nutanix.com
Wed Feb 19 02:39:22 UTC 2020



On 2/11/20, 7:06 PM, "Daniel P. Berrangé" <berrange at redhat.com> wrote:

    On Tue, Feb 11, 2020 at 10:05:53AM +0100, Martin Kletzander wrote:
  >  > On Wed, Feb 05, 2020 at 05:32:50PM +0000, Daniel P. Berrangé wrote:
    > > > On Mon, Feb 03, 2020 at 12:43:32PM +0000, Daniel P. Berrangé wrote:
    > > > > From: Shaju Abraham <shaju.abraham at nutanix.com>
    > > > > 
    > > > There are various config paths that a VM uses. The monitor paths and
    > > > > other lib paths are examples. These paths are tied to the VM name or
    > > > > UUID. The local migration breaks the assumption that there will be only
    > > > > one VM with a unique UUID and name. During local migrations there can be
    > > > > multiple VMs with same name and UUID in the same host. Append the
    > > > > domain-id field to the path so that there is no duplication of path
    > > > names.
    > > > 
    > > >This is the really critical problem with localhost migration.
    > > >
    > > >Appending the domain-id looks "simple" but this is a significant
    > > >behavioural / functional change for applications, and I don't think
    > > >it can fully solve the problem either.
    > > > 
    > > >This is changing thue paths used in various places where libvirt
    > > >internally generates unique paths (eg QMP socket, huge page or
    > > >file based memory paths, and defaults used for auto-filling
    > > >device paths (<channel> if not specified).
    > > >
    > > >Some of these paths are functionally important to external
    > > >applications and cannot be changed in this way. eg stuff
    > > >integrating with QEMU can be expecting certain memory backing
    > > >file paths, or certain <channel> paths & is liable to break
    > > >if we change the naming convention.
    > > >
    > > >For sake of argument, lets assume we can changing the naming
    > > >convention without breaking anything...
    > > >
    > >
    > >This was already done in (I would say) most places as they use
    > >virDomainDefGetShortName() to get a short, unique name of a directory -- it uses
    > >the domain ID as a prefix.
    > >
    > > > This only applies to paths libvirt generates at VM startup.
    > > >
    > > >There are plenty of configuration elements in the guest XML
    > > >that are end user / mgmt app defined, and reference paths in
    > > >the host OS.
    > > >
    > > >For example <graphics>, <serial>, <console>, <channel>,
    > > >all support UNIX domain sockets and TCP sockets. A UNIX
    > > >domain socket cannot be listened on by multiple VMs
    > > >at once. If the UNIX socket is in client mode, we cannot
    > > >assume the thing QEMU is connecting to allows multiple
    > > >concurrent connections. eg 2 QEMU's could have their
    > > ><serial> connected together over a UNIX socket pair.
    > > >Similarly if automatic TCP port assignment is not used
    > > >we cannot have multiple QEMU's listening on the same
    > > >host.
    > > >
    > > >One answer is to say that localhost migration is just
    > > >not supported for such VMs, but I don't find that very
    > > >convincing because the UNIX domain socket configs
    > > >affected are in common use.
    > > >
    > >
    > >I would be okay with saying that these either need to be changed in a provided
    > >destination XML or the migration will probably break.  I do not think it is
    > >unreasonable to say that if users are trying to shoot themselves, we should not
    > >spend a ridiculous time just so we can prevent that.  Otherwise we will get to
    > >the same point as we are now -- there might be a case where a local migration
    > >would work, but users cannot execute it even if they were very cautious and went
    > >through all the things that can prevent it from the technical point of view,
    > >libvirt will still disallow that.
    
    >If there are clashing resources, we can't rely on QEMU reporting an
    >error. For example with a UNIX domain socket, the first thing QEMU
    >does is unlink(/socket/path), which will blow away the UNIX domain
    >socket belonging to the original QEMU. As a result if migration
    >fails, and we rollback, the original QEMU will be broken.

   By appending the id field to the path, we are effectively nullifying this particular
   concern. Each qemu instance will get its own unique path and monitor. If a migration
   fails, we can roll back.
   I understand the need to keep the paths unchanged for externally generated configurations.
   How  about the below approach? Symlinks are created so that until the migration is complete
   the path remains unchanged for the  source . 
   I am still having issues with the emails, so pasting the code inline.

>From 37ead09491f1e4ae09f8bba1204ae4626390fb2d Mon Sep 17 00:00:00 2001
From: Shaju Abraham <shaju.abraham at nutanix.com>
Date: Wed, 22 Jan 2020 02:06:03 -0800
Subject: [PATCH 5/6] Make PATHs unique for a VM object instance

There are various config paths that a VM uses. The monitor paths and
other lib paths are examples. These paths are tied to the VM name or
UUID. The local migration breaks the assumption that there will be only
one VM with a unique UUID and name. During local migrations there can be
multiple VMs with same name and UUID in the same host. Append the
domain-id field to the path so that there is no duplication of path
names.

Signed-off-by: Shaju Abraham <shaju.abraham at nutanix.com>
---
 src/qemu/qemu_conf.c    |  4 ++--
 src/qemu/qemu_domain.c  | 40 +++++++++++++++++++++++++++++++++-------
 src/qemu/qemu_domain.h  |  4 ++++
 src/qemu/qemu_process.c |  6 ++++++
 4 files changed, 45 insertions(+), 9 deletions(-)

diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c
index b62dd1d..a2d55ce 100644
--- a/src/qemu/qemu_conf.c
+++ b/src/qemu/qemu_conf.c
@@ -1896,7 +1896,7 @@ qemuGetDomainHugepagePath(const virDomainDef *def,
     char *ret = NULL;
 
     if (base && domPath)
-        ret = g_strdup_printf("%s/%s", base, domPath);
+        ret = g_strdup_printf("%s/%s-%d", base, domPath, def->id);
     return ret;
 }
 
@@ -1964,7 +1964,7 @@ qemuGetMemoryBackingDomainPath(const virDomainDef *def,
         return -1;
 
     qemuGetMemoryBackingBasePath(cfg, &base);
-    *path = g_strdup_printf("%s/%s", base, shortName);
+    *path = g_strdup_printf("%s/%s-%d", base, shortName, def->id);
 
     return 0;
 }
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index ce0c5b7..9478501 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -2127,11 +2127,13 @@ qemuDomainSetPrivatePathsOld(virQEMUDriverPtr driver,
     virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver);
if (!priv->libDir)
-        priv->libDir = g_strdup_printf("%s/domain-%s", cfg->libDir, vm->def->name);
+        priv->libDir = g_strdup_printf("%s/domain-%s-%d", cfg->libDir,
+                                       vm->def->name, vm->def->id);
 
     if (!priv->channelTargetDir)
-        priv->channelTargetDir = g_strdup_printf("%s/domain-%s",
-                                                 cfg->channelTargetDir, vm->def->name);
+        priv->channelTargetDir = g_strdup_printf("%s/domain-%s-%d",
+                                                 cfg->channelTargetDir,
+                                                 vm->def->name, vm->def->id);
 
     virObjectUnref(cfg);
 }
@@ -2150,12 +2152,19 @@ qemuDomainSetPrivatePaths(virQEMUDriverPtr driver,
         goto cleanup;
 
     if (!priv->libDir)
-        priv->libDir = g_strdup_printf("%s/domain-%s", cfg->libDir, domname);
+        priv->libDir = g_strdup_printf("%s/domain-%s-%d", cfg->libDir, domname,
+                                       vm->def->id);
 
+    if (!priv->libDirsymLink)
+        priv->libDirsymLink = g_strdup_printf("%s/domain-%s", cfg->libDir, domname);
     if (!priv->channelTargetDir)
-        priv->channelTargetDir = g_strdup_printf("%s/domain-%s",
-                                                 cfg->channelTargetDir, domname);
-
+        priv->channelTargetDir = g_strdup_printf("%s/domain-%s-%d",
+                                                 cfg->channelTargetDir,
+                                                 domname, vm->def->id);
+    if (!priv->channelTargetDirsymLink)
+        priv->channelTargetDirsymLink = g_strdup_printf("%s/domain-%s",
+                                                        cfg->channelTargetDir,
+                                                        domname);
     ret = 0;
  cleanup:
     virObjectUnref(cfg);
@@ -2163,6 +2172,23 @@ qemuDomainSetPrivatePaths(virQEMUDriverPtr driver,
     return ret;
 }
 +int
+qemuDomainSymLinkPrivatePaths(virQEMUDriverPtr driver,
+                              virDomainObjPtr vm)
+{
+    virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver);
+    qemuDomainObjPrivatePtr priv = vm->privateData;
+    int ret = -1;
+    if (symlink(priv->libDir, priv->libDirsymLink ) != 0)
+        goto cleanup;
+    if (symlink(priv->channelTargetDir,priv->channelTargetDirsymLink ) != 0)
+        goto cleanup;
+    ret = 0;
+ cleanup:
+    virObjectUnref(cfg);
+    return ret;
+}
+
 
 static void
 dbusVMStateHashFree(void *opaque)
diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h
index 21ece23..7cdb16b 100644
--- a/src/qemu/qemu_domain.h
+++ b/src/qemu/qemu_domain.h
@@ -358,6 +358,8 @@ struct _qemuDomainObjPrivate {
     char *libDir;            /* base path for per-domain files */
     char *channelTargetDir;  /* base path for per-domain channel targets */
 
+    char *libDirsymLink;     /* base path for per-domain files for backward compatibility*/
+    char *channelTargetDirsymLink;  /* base path for per-domain channel targets for backward compatibility */
     /* random masterKey and length for encryption (not to be saved in our */
     /* private XML) - need to restore at process reconnect */
     uint8_t *masterKey;
@@ -1014,6 +1016,8 @@ bool qemuDomainNetSupportsMTU(virDomainNetType type);
 
 int qemuDomainSetPrivatePaths(virQEMUDriverPtr driver,
                               virDomainObjPtr vm);
+int qemuDomainSymLinkPrivatePaths(virQEMUDriverPtr driver,
+                                  virDomainObjPtr vm);
 
 virDomainDiskDefPtr qemuDomainDiskByName(virDomainDefPtr def, const char *name); 

diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index 9029fd1..1187275 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -6527,6 +6527,10 @@ qemuProcessPrepareHost(virQEMUDriverPtr driver,
     if (qemuProcessMakeDir(driver, vm, priv->libDir) < 0 ||
         qemuProcessMakeDir(driver, vm, priv->channelTargetDir) < 0)
         return -1;
+    if (!(flags & VIR_QEMU_PROCESS_START_LOCAL_MIG)) {
+        if (qemuDomainSymLinkPrivatePaths(driver, vm) < 0)
+            goto cleanup;
+    }
 
     VIR_DEBUG("Write domain masterKey");
     if (qemuDomainWriteMasterKeyFile(driver, vm) < 0)
@@ -7365,6 +7369,8 @@ void qemuProcessStop(virQEMUDriverPtr driver,
     virFileDeleteTree(priv->libDir);
     virFileDeleteTree(priv->channelTargetDir);
 
+    unlink(priv->libDirsymLink);
+    unlink(priv->channelTargetDirsymLink);
     ignore_value(virDomainChrDefForeach(vm->def,
                                         false,
                                         qemuProcessCleanupChardevDevice,
--
    
    >Preventing users from shooting themselves in the foot is a core
    >part of the value that libvirt is adding for QEMU migration. We
    >do this with stable device ABI, and controlled locking / disk
    >labelling, and CPU compatibility checking, and so on.
    
    >We're not perfect by any means, but the one thing we've tried
    >very hard to ensure is that if the destination QEMU fails for
    >any reason during migration, the user should always be able to
    >rollback to use the original source QEMU without problems.
    >The localhost migration support makes it harder to guarantee
    >the the source QEMU is not broken, so I do think we need to
    >make extra affect to protect users, if we're going to try to
    >allow this.
    
    >This series has taken the approach of trying to make the localhost
    >migration work as if it were just a normal remote migration, with
    >just the minimum change to alter some auto-generated paths on disk,
    >and keeping the second list of domains.
    
    >So we still the same  begin/prepare/perform/finish/confirm
    >phases fully separated. IOW, essentially the migration code has
    >very little, almost no, knowlege of the fact that this is a
    >localhost migration.
    
    >This is understandaable as a way to minimize the invasiveness
    >of any changes, but I think it misses the point that localhost
    >migration needs more than just changing a few paths on disk.
    
    
    
    > > Adding at least partial support where we could say we rely on QEMU failing
    > > reasonably would allow couple of mgmt apps to do more than they can do now.  And
    > >they might have taken care of the path collisions (e.g. when running libvirtd in
    > >containers or so).
    
    >If libvirtd is running inside a container, then from libvirt's
    >POV this is no longer localhost migration - it is regular
    >cross-host migration. The only caveat is that the container
    >must be reporting a unique hostname + UUID. We can at least
    >override UUID in libvirtd.conf if that isn't the case.
    
    > 
    > > If localhost migration is only usable in a small subset
    > > scenarios, I'm not convinced it is worth the support
    > > burden. Rarely used & tested features are liable to
    > > bitrot, and migration is already a serious point of
    > > instability in QEMU in general.
    
    
    Regards
    Shaju
    
    





More information about the libvir-list mailing list