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

Re: [libvirt] (Resend with updates) Live Migration with non-shared storage for kvm




>Eric Blake <eblake redhat com> wrote on 30/04/2010 18:26:04:

> From: Eric Blake <eblake redhat com>
> To: Cole Robinson <crobinso redhat com>
> Cc: Kenneth Nagin/Haifa/IBM IBMIL, list libvirt <libvir-list redhat com>
> Date: 30/04/2010 18:26
> Subject: Re: [libvirt] (Resend) Live Migration with non-shared storage
for kvm
>
> On 04/30/2010 06:42 AM, Cole Robinson wrote:
> > Other than that, the code generally looks like (except for the compiler
and
> > syntax-check warnings).
> >
> > - Cole
> >
> > (Here's the patch inline for the benefit of other reviewers)
>
> In addition to Cole's comments, here's some style nits that 'make
> syntax-check' won't pick up on...
>
> >> +++ b/src/qemu/qemu_driver.c
> >> @@ -4871,7 +4871,7 @@ static int qemudDomainSaveFlag(virDomainPtr
> dom, const char *path,
> >>      if (header.compressed == QEMUD_SAVE_FORMAT_RAW) {
> >>          const char *args[] = { "cat", NULL };
> >>          qemuDomainObjEnterMonitorWithDriver(driver, vm);
> >> -        rc = qemuMonitorMigrateToCommand(priv->mon, 1, args, path);
> >> +        rc = qemuMonitorMigrateToCommand(priv->mon,
> QEMU_MONITOR_MIGRATE_BACKGROUND, args, path);
>
> Where possible, wrap lines to fit 80 columns.  For example,
>
>     rc = qemuMonitorMigrateToCommand(priv->mon,
>                                      QEMU_MONITOR_MIGRATE_BACKGROUND,
>                                      args, path);
>
> >> -                           unsigned long flags ATTRIBUTE_UNUSED,
> >> +                           unsigned long flags ,
>
> s/flags ,/flags,/
>
> >>                             const char *dname ATTRIBUTE_UNUSED,
> >>                             unsigned long resource)
> >>  {
> >>      int ret = -1;
> >>      xmlURIPtr uribits = NULL;
> >>      qemuDomainObjPrivatePtr priv = vm->privateData;
> >> +    int background_flags = 0;
>
> Flags should generally be 'unsigned int', to make it less likely to
> cause problems with inadvertent sign extension if we ever reach 32 flags.
>
> >>
> >>      /* Issue the migrate command. */
> >>      if (STRPREFIX(uri, "tcp:") && !STRPREFIX(uri, "tcp://")) {
> >> @@ -9684,7 +9685,13 @@ static int doNativeMigrate(struct
> qemud_driver *driver,
> >>          goto cleanup;
> >>      }
> >>
> >> -    if (qemuMonitorMigrateToHost(priv->mon, 1, uribits->server,
> uribits->port) < 0) {
> >> +    if (flags & VIR_MIGRATE_NON_SHARED_DISK)
> >> +       background_flags |= QEMU_MONITOR_MIGRATE_NON_SHARED_DISK;
>
> That TAB will be caught by 'make syntax-check'.
>
> >> +++ b/src/qemu/qemu_monitor_text.c
> >> @@ -1132,18 +1132,19 @@ static int qemuMonitorTextMigrate
> (qemuMonitorPtr mon,
> >>      char *info = NULL;
> >>      int ret = -1;
> >>      char *safedest = qemuMonitorEscapeArg(dest);
> >> -    const char *extra;
> >> +    //const char *extra;
>
> No point leaving a dead comment; version-control is there to let us see
> what extra was declared as before your patch changed it to:
>
> >> +    const char extra[25] = " ";
>
> Why 25?  That seems like a magic number, and it wastes space compared to
> the maximum amount that you strcat() into it below.
>
> >>
> >>      if (!safedest) {
> >>          virReportOOMError();
> >>          return -1;
> >>      }
> >> -
> >> -    if (background)
> >> -        extra = "-d ";
> >> -    else
> >> -        extra = " ";
> >> -
> >> +    if (background & QEMU_MONITOR_MIGRATE_BACKGROUND)
> >> +       strcat(extra," -d");
> >> +    if (background & QEMU_MONITOR_MIGRATE_NON_SHARED_DISK)
> >> +        strcat(extra," -b");
> >> +    if (background & QEMU_MONITOR_MIGRATE_NON_SHARED_INC)
> >> +        strcat(extra," -i");
> >>      if (virAsprintf(&cmd, "migrate %s\"%s\"", extra, safedest) < 0) {
>
> That combination of strcat() and virAsprintf() looks bad.  In general,
> you should avoid strcat() (it often has overflow problems).  And since
> there is already allocation going on with virAsprintf, it seems like the
> better way to write this would be to convert both strcat and virAsprintf
> into multiple uses of the virBuffer* API, in order to build a single
> malloc'd string incrementally.
>
> --
> Eric Blake   eblake redhat com    +1-801-349-2682
> Libvirt virtualization library http://libvirt.org
>
> [attachment "signature.asc" deleted by Kenneth Nagin/Haifa/IBM]
I implemented the suggested changes by Cole and Eric, namely:
   replaced tabs with blanks (Cole)
   replace virsh flag names with copy-storage-all and copy-storage-inc
   (Cole)
   replaced strcat with virBuffer* API (Eric)
   replaced char extra[25] with virBuffer declaration (Eric)
   replace int background with unsigned int background (Eric)
I did not make changes to JSON (suggested by Cole) because I didn't know
how to verify the fixes.
I did not replace flags with unsigned int in qemuMonitorMigrateToCommand
(suggested by Eric) because these flags are part of the external interface
and past with rpcgen.  I worried that the change might create more
problems, and not worth the small benefit in style.

This the fix:
diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in
index f296d16..db107cc 100644
--- a/include/libvirt/libvirt.h.in
+++ b/include/libvirt/libvirt.h.in
@@ -411,6 +411,10 @@ typedef enum {
     VIR_MIGRATE_PERSIST_DEST      = (1 << 3), /* persist the VM on the
destination */
     VIR_MIGRATE_UNDEFINE_SOURCE   = (1 << 4), /* undefine the VM on the
source */
     VIR_MIGRATE_PAUSED            = (1 << 5), /* pause on remote side */
+    VIR_MIGRATE_NON_SHARED_DISK   = (1 << 6), /* migration with non-shared
storage with full disk copy */
+    VIR_MIGRATE_NON_SHARED_INC    = (1 << 7), /* migration with non-shared
storage with incremental copy */
+                                              /* (same base image shared
between source and destination) */
+
 } virDomainMigrateFlags;

 /* Domain migration. */
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 08cff00..9a3461d 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -4958,7 +4958,7 @@ static int qemudDomainSaveFlag(virDomainPtr dom,
const char *path,
     if (header.compressed == QEMUD_SAVE_FORMAT_RAW) {
         const char *args[] = { "cat", NULL };
         qemuDomainObjEnterMonitorWithDriver(driver, vm);
-        rc = qemuMonitorMigrateToFile(priv->mon, 1, args, path, offset);
+        rc = qemuMonitorMigrateToFile(priv->mon,
QEMU_MONITOR_MIGRATE_BACKGROUND, args, path, offset);
         qemuDomainObjExitMonitorWithDriver(driver, vm);
     } else {
         const char *prog = qemudSaveCompressionTypeToString
(header.compressed);
@@ -4968,7 +4968,7 @@ static int qemudDomainSaveFlag(virDomainPtr dom,
const char *path,
             NULL
         };
         qemuDomainObjEnterMonitorWithDriver(driver, vm);
-        rc = qemuMonitorMigrateToFile(priv->mon, 1, args, path, offset);
+        rc = qemuMonitorMigrateToFile(priv->mon,
QEMU_MONITOR_MIGRATE_BACKGROUND, args, path, offset);
         qemuDomainObjExitMonitorWithDriver(driver, vm);
     }

@@ -5286,9 +5286,8 @@ static int qemudDomainCoreDump(virDomainPtr dom,
     }

     qemuDomainObjEnterMonitorWithDriver(driver, vm);
-    ret = qemuMonitorMigrateToFile(priv->mon, 1, args, path, 0);
+    ret = qemuMonitorMigrateToFile(priv->mon,
QEMU_MONITOR_MIGRATE_BACKGROUND, args, path, 0);
     qemuDomainObjExitMonitorWithDriver(driver, vm);
-
     if (ret < 0)
         goto endjob;

@@ -9885,13 +9884,14 @@ cleanup:
 static int doNativeMigrate(struct qemud_driver *driver,
                            virDomainObjPtr vm,
                            const char *uri,
-                           unsigned long flags ATTRIBUTE_UNUSED,
+                           unsigned long flags ,
                            const char *dname ATTRIBUTE_UNUSED,
                            unsigned long resource)
 {
     int ret = -1;
     xmlURIPtr uribits = NULL;
     qemuDomainObjPrivatePtr priv = vm->privateData;
+    unsigned int background_flags = 0;

     /* Issue the migrate command. */
     if (STRPREFIX(uri, "tcp:") && !STRPREFIX(uri, "tcp://")) {
@@ -9919,7 +9919,13 @@ static int doNativeMigrate(struct qemud_driver
*driver,
         goto cleanup;
     }

-    if (qemuMonitorMigrateToHost(priv->mon, 1, uribits->server, uribits->
port) < 0) {
+    if (flags & VIR_MIGRATE_NON_SHARED_DISK)
+        background_flags |= QEMU_MONITOR_MIGRATE_NON_SHARED_DISK;
+
+    if (flags & VIR_MIGRATE_NON_SHARED_INC)
+        background_flags |= QEMU_MONITOR_MIGRATE_NON_SHARED_INC;
+
+    if (qemuMonitorMigrateToHost(priv->mon, background_flags, uribits->
server, uribits->port) < 0) {
         qemuDomainObjExitMonitorWithDriver(driver, vm);
         goto cleanup;
     }
@@ -10004,7 +10010,7 @@ static int doTunnelMigrate(virDomainPtr dom,
     unsigned long long qemuCmdFlags;
     int status;
     unsigned long long transferred, remaining, total;
-
+    unsigned int background_flags = QEMU_MONITOR_MIGRATE_BACKGROUND;
     /*
      * The order of operations is important here to avoid touching
      * the source VM until we are very sure we can successfully
@@ -10089,11 +10095,16 @@ static int doTunnelMigrate(virDomainPtr dom,

     /*   3. start migration on source */
     qemuDomainObjEnterMonitorWithDriver(driver, vm);
-    if (qemuCmdFlags & QEMUD_CMD_FLAG_MIGRATE_QEMU_UNIX)
-        internalret = qemuMonitorMigrateToUnix(priv->mon, 1, unixfile);
+    if (flags & VIR_MIGRATE_NON_SHARED_DISK)
+        background_flags |= QEMU_MONITOR_MIGRATE_NON_SHARED_DISK;
+    if (flags & VIR_MIGRATE_NON_SHARED_INC)
+        background_flags |= QEMU_MONITOR_MIGRATE_NON_SHARED_INC;
+    if (qemuCmdFlags & QEMUD_CMD_FLAG_MIGRATE_QEMU_UNIX){
+        internalret = qemuMonitorMigrateToUnix(priv->mon,
background_flags, unixfile);
+    }
     else if (qemuCmdFlags & QEMUD_CMD_FLAG_MIGRATE_QEMU_EXEC) {
         const char *args[] = { "nc", "-U", unixfile, NULL };
-        internalret = qemuMonitorMigrateToCommand(priv->mon, 1, args);
+        internalret = qemuMonitorMigrateToCommand(priv->mon,
QEMU_MONITOR_MIGRATE_BACKGROUND, args);
     } else {
         internalret = -1;
     }
diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c
index fca8590..443d216 100644
--- a/src/qemu/qemu_monitor.c
+++ b/src/qemu/qemu_monitor.c
@@ -1161,7 +1161,7 @@ int qemuMonitorGetMigrationStatus(qemuMonitorPtr mon,


 int qemuMonitorMigrateToHost(qemuMonitorPtr mon,
-                             int background,
+                             unsigned int background,
                              const char *hostname,
                              int port)
 {
@@ -1178,7 +1178,7 @@ int qemuMonitorMigrateToHost(qemuMonitorPtr mon,


 int qemuMonitorMigrateToCommand(qemuMonitorPtr mon,
-                                int background,
+                                unsigned int background,
                                 const char * const *argv)
 {
     int ret;
@@ -1193,7 +1193,7 @@ int qemuMonitorMigrateToCommand(qemuMonitorPtr mon,
 }

 int qemuMonitorMigrateToFile(qemuMonitorPtr mon,
-                             int background,
+                             unsigned int background,
                              const char * const *argv,
                              const char *target,
                              unsigned long long offset)
@@ -1217,7 +1217,7 @@ int qemuMonitorMigrateToFile(qemuMonitorPtr mon,
 }

 int qemuMonitorMigrateToUnix(qemuMonitorPtr mon,
-                             int background,
+                             unsigned int background,
                              const char *unixfile)
 {
     int ret;
diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h
index 3fa83b7..9760b4c 100644
--- a/src/qemu/qemu_monitor.h
+++ b/src/qemu/qemu_monitor.h
@@ -241,25 +241,32 @@ int qemuMonitorGetMigrationStatus(qemuMonitorPtr mon,
                                   unsigned long long *remaining,
                                   unsigned long long *total);

+typedef enum {
+  QEMU_MONITOR_MIGRATE_BACKGROUND        = 1 << 0,
+  QEMU_MONITOR_MIGRATE_NON_SHARED_DISK  = 1 << 1, /* migration with
non-shared storage with full disk copy */
+  QEMU_MONITOR_MIGRATE_NON_SHARED_INC   = 1 << 2, /* migration with
non-shared storage with incremental copy */
+  QEMU_MONITOR_MIGRATION_FLAGS_LAST
+} QEMU_MONITOR_MIGRATE;
+
 int qemuMonitorMigrateToHost(qemuMonitorPtr mon,
-                             int background,
+                             unsigned int background,
                              const char *hostname,
                              int port);

 int qemuMonitorMigrateToCommand(qemuMonitorPtr mon,
-                                int background,
+                                unsigned int background,
                                 const char * const *argv);

 # define QEMU_MONITOR_MIGRATE_TO_FILE_BS 512llu

 int qemuMonitorMigrateToFile(qemuMonitorPtr mon,
-                             int background,
+                             unsigned int background,
                              const char * const *argv,
                              const char *target,
                              unsigned long long offset);

 int qemuMonitorMigrateToUnix(qemuMonitorPtr mon,
-                             int background,
+                             unsigned int background,
                              const char *unixfile);

 int qemuMonitorMigrateCancel(qemuMonitorPtr mon);
diff --git a/src/qemu/qemu_monitor_text.c b/src/qemu/qemu_monitor_text.c
index 3f917bf..ff79663 100644
--- a/src/qemu/qemu_monitor_text.c
+++ b/src/qemu/qemu_monitor_text.c
@@ -38,6 +38,7 @@
 #include "driver.h"
 #include "datatypes.h"
 #include "virterror_internal.h"
+#include "buf.h"

 #define VIR_FROM_THIS VIR_FROM_QEMU

@@ -1125,26 +1126,32 @@ cleanup:


 static int qemuMonitorTextMigrate(qemuMonitorPtr mon,
-                                  int background,
+                                  unsigned int background,
                                   const char *dest)
 {
     char *cmd = NULL;
     char *info = NULL;
     int ret = -1;
     char *safedest = qemuMonitorEscapeArg(dest);
-    const char *extra;
+    virBuffer extra = VIR_BUFFER_INITIALIZER;

     if (!safedest) {
         virReportOOMError();
         return -1;
     }
-
-    if (background)
-        extra = "-d ";
-    else
-        extra = " ";
-
-    if (virAsprintf(&cmd, "migrate %s\"%s\"", extra, safedest) < 0) {
+    virBufferAddLit(&extra, " ");
+    if (background & QEMU_MONITOR_MIGRATE_BACKGROUND)
+        virBufferAddLit(&extra, " -d");
+    if (background & QEMU_MONITOR_MIGRATE_NON_SHARED_DISK)
+        virBufferAddLit(&extra, " -b");
+    if (background & QEMU_MONITOR_MIGRATE_NON_SHARED_INC)
+        virBufferAddLit(&extra, " -i");
+    if (virBufferError(&extra)) {
+        virBufferFreeAndReset(&extra);
+        virReportOOMError();
+        return -1;
+    }
+    if (virAsprintf(&cmd, "migrate %s\"%s\"", virBufferContentAndReset
(&extra), safedest) < 0) {
         virReportOOMError();
         goto cleanup;
     }
@@ -1180,7 +1187,7 @@ cleanup:
 }

 int qemuMonitorTextMigrateToHost(qemuMonitorPtr mon,
-                                 int background,
+                                 unsigned int background,
                                  const char *hostname,
                                  int port)
 {
@@ -1201,7 +1208,7 @@ int qemuMonitorTextMigrateToHost(qemuMonitorPtr mon,


 int qemuMonitorTextMigrateToCommand(qemuMonitorPtr mon,
-                                    int background,
+                                    unsigned int background,
                                     const char * const *argv)
 {
     char *argstr;
@@ -1228,7 +1235,7 @@ cleanup:
 }

 int qemuMonitorTextMigrateToFile(qemuMonitorPtr mon,
-                                 int background,
+                                 unsigned int background,
                                  const char * const *argv,
                                  const char *target,
                                  unsigned long long offset)
@@ -1269,7 +1276,7 @@ cleanup:
 }

 int qemuMonitorTextMigrateToUnix(qemuMonitorPtr mon,
-                                 int background,
+                                 unsigned int background,
                                  const char *unixfile)
 {
     char *dest = NULL;
diff --git a/src/qemu/qemu_monitor_text.h b/src/qemu/qemu_monitor_text.h
index 23c3a45..7d1bc56 100644
--- a/src/qemu/qemu_monitor_text.h
+++ b/src/qemu/qemu_monitor_text.h
@@ -93,22 +93,22 @@ int qemuMonitorTextGetMigrationStatus(qemuMonitorPtr
mon,
                                       unsigned long long *total);

 int qemuMonitorTextMigrateToHost(qemuMonitorPtr mon,
-                                 int background,
+                                 unsigned int background,
                                  const char *hostname,
                                  int port);

 int qemuMonitorTextMigrateToCommand(qemuMonitorPtr mon,
-                                    int background,
+                                    unsigned int background,
                                     const char * const *argv);

 int qemuMonitorTextMigrateToFile(qemuMonitorPtr mon,
-                                 int background,
+                                 unsigned int background,
                                  const char * const *argv,
                                  const char *target,
                                  unsigned long long offset);

 int qemuMonitorTextMigrateToUnix(qemuMonitorPtr mon,
-                                 int background,
+                                 unsigned int background,
                                  const char *unixfile);

 int qemuMonitorTextMigrateCancel(qemuMonitorPtr mon);
diff --git a/tools/virsh.c b/tools/virsh.c
index eb11a78..f96d31a 100644
--- a/tools/virsh.c
+++ b/tools/virsh.c
@@ -2851,6 +2851,8 @@ static const vshCmdOptDef opts_migrate[] = {
     {"persistent", VSH_OT_BOOL, 0, N_("persist VM on destination")},
     {"undefinesource", VSH_OT_BOOL, 0, N_("undefine VM on source")},
     {"suspend", VSH_OT_BOOL, 0, N_("do not restart the domain on the
destination host")},
+    {"copy-storage-all", VSH_OT_BOOL, 0, N_("migration with non-shared
storage with full disk copy")},
+    {"copy-storage-inc", VSH_OT_BOOL, 0, N_("migration with non-shared
storage with incremental copy (same base image shared between source and
destination)")},
     {"domain", VSH_OT_DATA, VSH_OFLAG_REQ, N_("domain name, id or uuid")},
     {"desturi", VSH_OT_DATA, VSH_OFLAG_REQ, N_("connection URI of the
destination host")},
     {"migrateuri", VSH_OT_DATA, 0, N_("migration URI, usually can be
omitted")},
@@ -2898,6 +2900,12 @@ cmdMigrate (vshControl *ctl, const vshCmd *cmd)
     if (vshCommandOptBool (cmd, "suspend"))
         flags |= VIR_MIGRATE_PAUSED;

+    if (vshCommandOptBool (cmd, "copy-storage-all"))
+        flags |= VIR_MIGRATE_NON_SHARED_DISK;
+
+    if (vshCommandOptBool (cmd, "copy-storage-inc"))
+        flags |= VIR_MIGRATE_NON_SHARED_INC;
+
     if ((flags & VIR_MIGRATE_PEER2PEER) ||
         vshCommandOptBool (cmd, "direct")) {
         /* For peer2peer migration or direct migration we only expect one
URI


This the patch file:
(See attached file: libvirt_migration_ns_100503.patch)

regards,
Kenneth Nagin

Attachment: libvirt_migration_ns_100503.patch
Description: Binary data


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