[libvirt] Live Migration with non-shared storage for kvm
On Wed Apr 07 I sent an email with a patch titled Live Migration with non-shared storage for kvm. What is the status on the patch? Kenneth Nagin Ph: +972-4-8296227 Cell: 054-6976227 Fx: +972-4- 8296113 "Do not look back in anger, or forward in fear, but around in awareness." --James Thurber
On 04/19/2010 01:38 AM, Kenneth Nagin wrote:
On Wed Apr 07 I sent an email with a patch titled Live Migration with non-shared storage for kvm. What is the status on the patch?
If it didn't get an adequate response, it's probably a good idea to rebase the patch against latest code, and resend to the list: people may have deleted the old message. - Cole
Cole Robinson <crobinso@redhat.com> wrote on 20/04/2010 19:10:10:
From: Cole Robinson <crobinso@redhat.com> To: Kenneth Nagin/Haifa/IBM@IBMIL Cc: list libvirt <libvir-list@redhat.com> Date: 20/04/2010 19:10 Subject: Re: [libvirt] Live Migration with non-shared storage for kvm
On 04/19/2010 01:38 AM, Kenneth Nagin wrote:
On Wed Apr 07 I sent an email with a patch titled Live Migration with non-shared storage for kvm. What is the status on the patch?
If it didn't get an adequate response, it's probably a good idea to rebase the patch against latest code, and resend to the list: people may have deleted the old message.
- Cole
This is the second time I have been asked to rebase. I submitted the same patch in February. Daniel Veillard asked me to rebase that patch. I got very busy so I delayed submitting the patch until now. If necessary I'll submit again, but I'd like some assurance that I'm not just wasting my and your time. -- Kenneth Nagin
On 04/21/2010 03:27 AM, Kenneth Nagin wrote:
Cole Robinson <crobinso@redhat.com> wrote on 20/04/2010 19:10:10:
From: Cole Robinson <crobinso@redhat.com> To: Kenneth Nagin/Haifa/IBM@IBMIL Cc: list libvirt <libvir-list@redhat.com> Date: 20/04/2010 19:10 Subject: Re: [libvirt] Live Migration with non-shared storage for kvm
On 04/19/2010 01:38 AM, Kenneth Nagin wrote:
On Wed Apr 07 I sent an email with a patch titled Live Migration with non-shared storage for kvm. What is the status on the patch?
If it didn't get an adequate response, it's probably a good idea to rebase the patch against latest code, and resend to the list: people may have deleted the old message.
- Cole
This is the second time I have been asked to rebase. I submitted the same patch in February. Daniel Veillard asked me to rebase that patch. I got very busy so I delayed submitting the patch until now. If necessary I'll submit again, but I'd like some assurance that I'm not just wasting my and your time.
If you rebase and resend the patch, I'll review it. But seriously, everyone is always busy, and sometimes things slip through the cracks. If in the future you want to call attention to a forgotten patch, either rebase and resend with [RESEND] in the subject, or reply to the original patch submission with a 'ping' or similar, keeping the original mail intact so people don't have to check the archives to find the mail you are talking about. - Cole
Support for live migration between hosts that do not share storage was added to qemu-kvm release 0.12.1. It supports two flags: -b migration without shared storage with full disk copy -i migration without shared storage with incremental copy (same base image shared between source and destination). In order to support kvm's live migration with non-shared storage I added two migration flags that user can invoke: 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) */ Likewise I add complementary flags to virsh migrate: non_shared_disk and non_shared_inc As Daniel B. suggested I also added internal flags to be passed in the "background" parameter to the qemu monitor: 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 }; I updated qemu_driver.c's doNativeMigrate and doTunnelMigrate to map the external flags to the internal flags. I updated qemu_monitor_text's qemuMonitorTextMigrate to map the internal flags to the qemu monitor's command line flags. Also, qemudDomainSave ends up calling qemuMonitorTextMigrate, but it didn't support the external flags so I assumed that it was not relevant. I tested the live migration without shared storage (both flags) for native and p2p with and without tunnelling. I also verified that the fix doesn't affect normal migration with shared storage. Here is the diff patch file from the current git: (See attached file: libvirt_migration_ns_100422.patch) -- Kenneth Nagin
Cole Robinson <crobinso@redhat.com> wrote on 21/04/2010 16:57:29:
From: Cole Robinson <crobinso@redhat.com> To: Kenneth Nagin/Haifa/IBM@IBMIL Cc: Daniel Veillard <veillard@redhat.com>, "Daniel P. Berrange" <berrange@redhat.com>, list libvirt <libvir-list@redhat.com> Date: 21/04/2010 16:57 Subject: Re: [libvirt] Live Migration with non-shared storage for kvm
On 04/21/2010 03:27 AM, Kenneth Nagin wrote:
Cole Robinson <crobinso@redhat.com> wrote on 20/04/2010 19:10:10:
From: Cole Robinson <crobinso@redhat.com> To: Kenneth Nagin/Haifa/IBM@IBMIL Cc: list libvirt <libvir-list@redhat.com> Date: 20/04/2010 19:10 Subject: Re: [libvirt] Live Migration with non-shared storage for kvm
On 04/19/2010 01:38 AM, Kenneth Nagin wrote:
On Wed Apr 07 I sent an email with a patch titled Live Migration
with
non-shared storage for kvm. What is the status on the patch?
If it didn't get an adequate response, it's probably a good idea to rebase the patch against latest code, and resend to the list: people may have deleted the old message.
- Cole
This is the second time I have been asked to rebase. I submitted the same patch in February. Daniel Veillard asked me to rebase that patch. I got very busy so I delayed submitting the patch until now. If necessary I'll submit again, but I'd like some assurance that I'm not just wasting my and your time.
If you rebase and resend the patch, I'll review it.
But seriously, everyone is always busy, and sometimes things slip through the cracks. If in the future you want to call attention to a forgotten patch, either rebase and resend with [RESEND] in the subject, or reply to the original patch submission with a 'ping' or similar, keeping the original mail intact so people don't have to check the archives to find the mail you are talking about.
- Cole
On 04/22/2010 11:06 AM, Kenneth Nagin wrote:
Support for live migration between hosts that do not share storage was added to qemu-kvm release 0.12.1. It supports two flags: -b migration without shared storage with full disk copy -i migration without shared storage with incremental copy (same base image shared between source and destination).
In order to support kvm's live migration with non-shared storage I added two migration flags that user can invoke:
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) */ Likewise I add complementary flags to virsh migrate: non_shared_disk and non_shared_inc
As Daniel B. suggested I also added internal flags to be passed in the "background" parameter to the qemu monitor: 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 };
I updated qemu_driver.c's doNativeMigrate and doTunnelMigrate to map the external flags to the internal flags. I updated qemu_monitor_text's qemuMonitorTextMigrate to map the internal flags to the qemu monitor's command line flags.
Also, qemudDomainSave ends up calling qemuMonitorTextMigrate, but it didn't support the external flags so I assumed that it was not relevant.
I tested the live migration without shared storage (both flags) for native and p2p with and without tunnelling. I also verified that the fix doesn't affect normal migration with shared storage.
Here is the diff patch file from the current git:
(See attached file: libvirt_migration_ns_100422.patch)
-- Kenneth Nagin
Of course, I tell you to resend then forget to review. Sorry :( Applying the patch (to last weeks checkout), there are some compiler warnings: make sure you configure with --enable-compiler-warnings=error. 'make syntax-check' also fails, so please address these. Also, changes are probably needed to qemu_monitor_json.c to enable this feature for future qemu releases. Probably a simple change, but I don't know what it should be. Maybe Danpb can chime in here. Finding a way to post the patch in-line will also probably get better attention: just pasting it into the mail client will probably mangle the patch, I'd recommend git send-email. For the virsh changes, I wouldn't use underscores in cli flag names. Maybe change this to 'copy-storage' and 'increment-storage'. Not too fond of the latter, maybe others have ideas. 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)
diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in index 4addc62..28fabd9 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 5f4adfd..521638c 100644 --- a/src/qemu/qemu_driver.c +++ 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); qemuDomainObjExitMonitorWithDriver(driver, vm); } else { const char *prog = qemudSaveCompressionTypeToString(header.compressed); @@ -4881,7 +4881,7 @@ static int qemudDomainSaveFlag(virDomainPtr dom, const char *path, NULL }; qemuDomainObjEnterMonitorWithDriver(driver, vm); - rc = qemuMonitorMigrateToCommand(priv->mon, 1, args, path); + rc = qemuMonitorMigrateToCommand(priv->mon, QEMU_MONITOR_MIGRATE_BACKGROUND, args, path); qemuDomainObjExitMonitorWithDriver(driver, vm); }
@@ -5173,7 +5173,7 @@ static int qemudDomainCoreDump(virDomainPtr dom, }
qemuDomainObjEnterMonitor(vm); - ret = qemuMonitorMigrateToCommand(priv->mon, 1, args, path); + ret = qemuMonitorMigrateToCommand(priv->mon, QEMU_MONITOR_MIGRATE_BACKGROUND, args, path); qemuDomainObjExitMonitor(vm);
if (ret < 0) @@ -9650,13 +9650,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; + int background_flags = 0;
/* 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; + + 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; } @@ -9769,7 +9776,7 @@ static int doTunnelMigrate(virDomainPtr dom, unsigned long long qemuCmdFlags; int status; unsigned long long transferred, remaining, total; - + 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 @@ -9854,11 +9861,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, "/dev/null"); + internalret = qemuMonitorMigrateToCommand(priv->mon, background_flags, args, "/dev/null"); } else { internalret = -1; } diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h index 251233a..40a21f5 100644 --- a/src/qemu/qemu_monitor.h +++ b/src/qemu/qemu_monitor.h @@ -239,6 +239,13 @@ 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 +}; + int qemuMonitorMigrateToHost(qemuMonitorPtr mon, int background, const char *hostname, diff --git a/src/qemu/qemu_monitor_text.c b/src/qemu/qemu_monitor_text.c index 6ad07b1..136ac12 100644 --- a/src/qemu/qemu_monitor_text.c +++ 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; + const char extra[25] = " ";
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) { virReportOOMError(); goto cleanup; diff --git a/tools/virsh.c b/tools/virsh.c index b2a1538..bd8719e 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -2804,6 +2804,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")}, + {"non_shared_disk", VSH_OT_BOOL, 0, N_("migration with non-shared storage with full disk copy")}, + {"non_shared_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")},
@@ -2851,6 +2853,12 @@ cmdMigrate (vshControl *ctl, const vshCmd *cmd) if (vshCommandOptBool (cmd, "suspend")) flags |= VIR_MIGRATE_PAUSED;
+ if (vshCommandOptBool (cmd, "non_shared_disk")) + flags |= VIR_MIGRATE_NON_SHARED_DISK; + + if (vshCommandOptBool (cmd, "non_shared_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
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
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
On 05/03/2010 07:52 AM, Cole Robinson wrote:
On 05/03/2010 04:13 AM, Kenneth Nagin wrote:
...
This the patch file: (See attached file: libvirt_migration_ns_100503.patch)
ACK, patch looks fine now.
Kenneth, I added a commit message, broke a few lines to fit in 80 columns, added a virCheckFlags to doNativeMigrate (which required a type change), and removed an extra space in the generated line: diff --git i/src/qemu/qemu_driver.c w/src/qemu/qemu_driver.c index 5823e10..47ae52c 100644 --- i/src/qemu/qemu_driver.c +++ w/src/qemu/qemu_driver.c @@ -4958,7 +4958,9 @@ 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, QEMU_MONITOR_MIGRATE_BACKGROUND, 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 +4970,9 @@ static int qemudDomainSaveFlag(virDomainPtr dom, const char *path, NULL }; qemuDomainObjEnterMonitorWithDriver(driver, vm); - rc = qemuMonitorMigrateToFile(priv->mon, QEMU_MONITOR_MIGRATE_BACKGROUND, args, path, offset); + rc = qemuMonitorMigrateToFile(priv->mon, + QEMU_MONITOR_MIGRATE_BACKGROUND, + args, path, offset); qemuDomainObjExitMonitorWithDriver(driver, vm); } @@ -5286,7 +5290,9 @@ static int qemudDomainCoreDump(virDomainPtr dom, } qemuDomainObjEnterMonitorWithDriver(driver, vm); - ret = qemuMonitorMigrateToFile(priv->mon, QEMU_MONITOR_MIGRATE_BACKGROUND, args, path, 0); + ret = qemuMonitorMigrateToFile(priv->mon, + QEMU_MONITOR_MIGRATE_BACKGROUND, + args, path, 0); qemuDomainObjExitMonitorWithDriver(driver, vm); if (ret < 0) goto endjob; @@ -9884,7 +9890,7 @@ cleanup: static int doNativeMigrate(struct qemud_driver *driver, virDomainObjPtr vm, const char *uri, - unsigned long flags , + unsigned int flags, const char *dname ATTRIBUTE_UNUSED, unsigned long resource) { @@ -9893,6 +9899,9 @@ static int doNativeMigrate(struct qemud_driver *driver, qemuDomainObjPrivatePtr priv = vm->privateData; unsigned int background_flags = 0; + virCheckFlags(VIR_MIGRATE_NON_SHARED_DISK | VIR_MIGRATE_NON_SHARED_INC, + -1); + /* Issue the migrate command. */ if (STRPREFIX(uri, "tcp:") && !STRPREFIX(uri, "tcp://")) { /* HACK: source host generates bogus URIs, so fix them up */ @@ -9925,7 +9934,8 @@ static int doNativeMigrate(struct qemud_driver *driver, 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) { + if (qemuMonitorMigrateToHost(priv->mon, background_flags, uribits->server, + uribits->port) < 0) { qemuDomainObjExitMonitorWithDriver(driver, vm); goto cleanup; } @@ -10011,6 +10021,7 @@ static int doTunnelMigrate(virDomainPtr dom, 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 @@ -10100,7 +10111,8 @@ static int doTunnelMigrate(virDomainPtr dom, 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); + internalret = qemuMonitorMigrateToUnix(priv->mon, background_flags, + unixfile); } else if (qemuCmdFlags & QEMUD_CMD_FLAG_MIGRATE_QEMU_EXEC) { const char *args[] = { "nc", "-U", unixfile, NULL }; diff --git i/src/qemu/qemu_monitor_text.c w/src/qemu/qemu_monitor_text.c index ff79663..3119600 100644 --- i/src/qemu/qemu_monitor_text.c +++ w/src/qemu/qemu_monitor_text.c @@ -1139,7 +1139,7 @@ static int qemuMonitorTextMigrate(qemuMonitorPtr mon, virReportOOMError(); return -1; } - virBufferAddLit(&extra, " "); + if (background & QEMU_MONITOR_MIGRATE_BACKGROUND) virBufferAddLit(&extra, " -d"); if (background & QEMU_MONITOR_MIGRATE_NON_SHARED_DISK) Then I verified that it passes 'make check' and 'make syntax-check', and finally pushed this in your name. Thanks again for the contribution, and for the reminders to review it. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
Ph: +972-4-8296227 Cell: 054-6976227 Fx: +972-4- 8296113 "Do not look back in anger, or forward in fear, but around in awareness." --James Thurber Eric Blake <eblake@redhat.com> wrote on 05/05/2010 01:04:23:
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: 05/05/2010 01:04 Subject: Re: [libvirt] (Resend with updates) Live Migration with non-shared storage for kvm
On 05/03/2010 07:52 AM, Cole Robinson wrote:
On 05/03/2010 04:13 AM, Kenneth Nagin wrote:
...
This the patch file: (See attached file: libvirt_migration_ns_100503.patch)
ACK, patch looks fine now.
Kenneth,
I added a commit message, broke a few lines to fit in 80 columns, added a virCheckFlags to doNativeMigrate (which required a type change), and removed an extra space in the generated line:
diff --git i/src/qemu/qemu_driver.c w/src/qemu/qemu_driver.c index 5823e10..47ae52c 100644 --- i/src/qemu/qemu_driver.c +++ w/src/qemu/qemu_driver.c @@ -4958,7 +4958,9 @@ 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, QEMU_MONITOR_MIGRATE_BACKGROUND, 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 +4970,9 @@ static int qemudDomainSaveFlag(virDomainPtr dom, const char *path, NULL }; qemuDomainObjEnterMonitorWithDriver(driver, vm); - rc = qemuMonitorMigrateToFile(priv->mon, QEMU_MONITOR_MIGRATE_BACKGROUND, args, path, offset); + rc = qemuMonitorMigrateToFile(priv->mon, + QEMU_MONITOR_MIGRATE_BACKGROUND, + args, path, offset); qemuDomainObjExitMonitorWithDriver(driver, vm); }
@@ -5286,7 +5290,9 @@ static int qemudDomainCoreDump(virDomainPtr dom, }
qemuDomainObjEnterMonitorWithDriver(driver, vm); - ret = qemuMonitorMigrateToFile(priv->mon, QEMU_MONITOR_MIGRATE_BACKGROUND, args, path, 0); + ret = qemuMonitorMigrateToFile(priv->mon, + QEMU_MONITOR_MIGRATE_BACKGROUND, + args, path, 0); qemuDomainObjExitMonitorWithDriver(driver, vm); if (ret < 0) goto endjob; @@ -9884,7 +9890,7 @@ cleanup: static int doNativeMigrate(struct qemud_driver *driver, virDomainObjPtr vm, const char *uri, - unsigned long flags , + unsigned int flags, const char *dname ATTRIBUTE_UNUSED, unsigned long resource) { @@ -9893,6 +9899,9 @@ static int doNativeMigrate(struct qemud_driver *driver, qemuDomainObjPrivatePtr priv = vm->privateData; unsigned int background_flags = 0;
+ virCheckFlags(VIR_MIGRATE_NON_SHARED_DISK | VIR_MIGRATE_NON_SHARED_INC, + -1); + /* Issue the migrate command. */ if (STRPREFIX(uri, "tcp:") && !STRPREFIX(uri, "tcp://")) { /* HACK: source host generates bogus URIs, so fix them up */ @@ -9925,7 +9934,8 @@ static int doNativeMigrate(struct qemud_driver *driver, 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) { + if (qemuMonitorMigrateToHost(priv->mon, background_flags, uribits->server, + uribits->port) < 0) { qemuDomainObjExitMonitorWithDriver(driver, vm); goto cleanup; } @@ -10011,6 +10021,7 @@ static int doTunnelMigrate(virDomainPtr dom, 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 @@ -10100,7 +10111,8 @@ static int doTunnelMigrate(virDomainPtr dom, 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); + internalret = qemuMonitorMigrateToUnix(priv->mon, background_flags, + unixfile); } else if (qemuCmdFlags & QEMUD_CMD_FLAG_MIGRATE_QEMU_EXEC) { const char *args[] = { "nc", "-U", unixfile, NULL }; diff --git i/src/qemu/qemu_monitor_text.c w/src/qemu/qemu_monitor_text.c index ff79663..3119600 100644 --- i/src/qemu/qemu_monitor_text.c +++ w/src/qemu/qemu_monitor_text.c @@ -1139,7 +1139,7 @@ static int qemuMonitorTextMigrate(qemuMonitorPtr mon, virReportOOMError(); return -1; } - virBufferAddLit(&extra, " "); + if (background & QEMU_MONITOR_MIGRATE_BACKGROUND) virBufferAddLit(&extra, " -d"); if (background & QEMU_MONITOR_MIGRATE_NON_SHARED_DISK)
Then I verified that it passes 'make check' and 'make syntax-check', and finally pushed this in your name. Thanks again for the contribution, and for the reminders to review it.
-- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
[attachment "signature.asc" deleted by Kenneth Nagin/Haifa/IBM]
Thanks both of you for your assistance and thorough reviews. Since this is my first contribution, what happens next? How do I know whether it was accepted into an official release? regards, Kenneth Nagin
On 05/05/2010 12:26 AM, Kenneth Nagin wrote:
Eric Blake <eblake@redhat.com> wrote on 05/05/2010 01:04:23:
[feel free to trim content unrelated to your reply, it makes it easier to get to your comments]
Then I verified that it passes 'make check' and 'make syntax-check', and finally pushed this in your name. Thanks again for the contribution, and for the reminders to review it.
-- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
[attachment "signature.asc" deleted by Kenneth Nagin/Haifa/IBM]
Thanks both of you for your assistance and thorough reviews. Since this is my first contribution, what happens next? How do I know whether it was accepted into an official release?
It's already been applied in the upstream git repository. Which means that the next libvirt release (probably 0.8.2, and hopefully at the end of May per our typical release schedule) will include your changes. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
When is libvirt 0.8.2 expected to be released? Kenneth Nagin From: Eric Blake <eblake@redhat.com> To: Kenneth Nagin/Haifa/IBM@IBMIL Cc: list libvirt <libvir-list@redhat.com> Date: 05/05/2010 18:24 Subject: Re: [libvirt] (Resend with updates) Live Migration with non-shared storage for kvm On 05/05/2010 12:26 AM, Kenneth Nagin wrote:
Eric Blake <eblake@redhat.com> wrote on 05/05/2010 01:04:23:
[feel free to trim content unrelated to your reply, it makes it easier to get to your comments]
Then I verified that it passes 'make check' and 'make syntax-check', and finally pushed this in your name. Thanks again for the contribution, and for the reminders to review it.
-- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
[attachment "signature.asc" deleted by Kenneth Nagin/Haifa/IBM]
Thanks both of you for your assistance and thorough reviews. Since this is my first contribution, what happens next? How do I know whether it was accepted into an official release?
It's already been applied in the upstream git repository. Which means that the next libvirt release (probably 0.8.2, and hopefully at the end of May per our typical release schedule) will include your changes. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org [attachment "signature.asc" deleted by Kenneth Nagin/Haifa/IBM]
On Mon, May 03, 2010 at 09:52:10AM -0400, Cole Robinson wrote:
On 05/03/2010 04:13 AM, Kenneth Nagin wrote:
...
This the patch file: (See attached file: libvirt_migration_ns_100503.patch)
ACK, patch looks fine now.
No objections to the patch that was committed, but FYI there is an improvement that can be done to it. If you run 'virsh domjobinfo $GUEST' while migration is running it tells you the progress of migration. It gives overall progress, and memory progress. The APi was designed to allow us to report storage progress too. To wire this up look at the method qemuDomainWaitForMigrationComplete where it updates the priv->jobInfo variables. You'll also need to add extra params to the qemuMonitorGetMigrationStatus() method to return the disk processed/remaining/total stats virsh should already be able to report this data once the QEMU driver is wired up Regards, Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://deltacloud.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
"Daniel P. Berrange" <berrange@redhat.com> wrote on 05/05/2010 11:16:37:
From: "Daniel P. Berrange" <berrange@redhat.com> To: Cole Robinson <crobinso@redhat.com> Cc: Kenneth Nagin/Haifa/IBM@IBMIL, list libvirt <libvir-list@redhat.com> Date: 05/05/2010 11:16 Subject: Re: [libvirt] (Resend with updates) Live Migration with non-shared storage for kvm
On Mon, May 03, 2010 at 09:52:10AM -0400, Cole Robinson wrote:
On 05/03/2010 04:13 AM, Kenneth Nagin wrote:
...
This the patch file: (See attached file: libvirt_migration_ns_100503.patch)
ACK, patch looks fine now.
No objections to the patch that was committed, but FYI there is an improvement that can be done to it.
If you run 'virsh domjobinfo $GUEST' while migration is running it tells you the progress of migration. It gives overall progress, and memory progress. The APi was designed to allow us to report storage progress too. To wire this up look at the method
qemuDomainWaitForMigrationComplete
where it updates the priv->jobInfo variables. You'll also need to add extra params to the qemuMonitorGetMigrationStatus() method to return the disk processed/remaining/total stats
virsh should already be able to report this data once the QEMU driver is wired up
Regards, Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/:| |: http://libvirt.org -o- http://virt-manager.org -o- http://deltacloud.org:| |: http://autobuild.org -o- http://search.cpan.org/~danberr/:| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
Thank you. I will look into implementing the improvement when I have time. However, it won't be for a while since I have some pressing deadlines that I have to meet in the next few months. regards, Kenneth Nagin Ph: +972-4-8296227 Cell: 054-6976227 Fx: +972-4- 8296113 http://researcher.watson.ibm.com/researcher/view.php?person=il-NAGIN http://www.reservoir-fp7.eu/
Cole Robinson <crobinso@redhat.com> wrote on 30/04/2010 15:42:05:
From: Cole Robinson <crobinso@redhat.com> To: Kenneth Nagin/Haifa/IBM@IBMIL Cc: "Daniel P. Berrange" <berrange@redhat.com>, list libvirt <libvir-list@redhat.com>, Daniel Veillard <veillard@redhat.com> Date: 30/04/2010 15:42 Subject: Re: [libvirt] (Resend) Live Migration with non-shared storage for kvm
Applying the patch (to last weeks checkout), there are some compilerwarnings: make sure you configure with --enable-compiler-warnings=error. 'make syntax-check' also fails, so please address these.
But I get this error message when compiling with 'make syntax-check': 2.14 unmarked_diagnostics vulnerable_makefile_CVE-2009-4029 ./Makefile.in:1283: -find $(distdir) -type d ! -perm -777 -exec chmod a+rwx {} \; -o \ maint.mk: the above files are vulnerable; beware of running "make dist*" rules, and upgrade to fixed automake see http://bugzilla.redhat.com/542609 for details make: *** [sc_vulnerable_makefile_CVE-2009-4029] Error 1 This problem is unrelated to any changes that I made and appearantly the compile completes because make install works properly. Any suggestions on how to resolve this error message. regards, Kenneth Nagin
Kenneth Nagin wrote:
Cole Robinson <crobinso@redhat.com> wrote on 30/04/2010 15:42:05:
From: Cole Robinson <crobinso@redhat.com> To: Kenneth Nagin/Haifa/IBM@IBMIL Cc: "Daniel P. Berrange" <berrange@redhat.com>, list libvirt <libvir-list@redhat.com>, Daniel Veillard <veillard@redhat.com> Date: 30/04/2010 15:42 Subject: Re: [libvirt] (Resend) Live Migration with non-shared storage for kvm
Applying the patch (to last weeks checkout), there are some compilerwarnings: make sure you configure with --enable-compiler-warnings=error. 'make syntax-check' also fails, so please address these.
But I get this error message when compiling with 'make syntax-check':
2.14 unmarked_diagnostics vulnerable_makefile_CVE-2009-4029 ./Makefile.in:1283: -find $(distdir) -type d ! -perm -777 -exec chmod a+rwx {} \; -o \ maint.mk: the above files are vulnerable; beware of running "make dist*" rules, and upgrade to fixed automake see http://bugzilla.redhat.com/542609 for details make: *** [sc_vulnerable_makefile_CVE-2009-4029] Error 1
This problem is unrelated to any changes that I made and appearantly the compile completes because make install works properly.
Any suggestions on how to resolve this error message.
That means you are using a version of automake that lacks the fix for the referenced bug. Upgrading to a patched version of automake, and regenerating all Makefile.in files will fix it. If you run any make rule that runs that find command, you may expose yourself to a nasty exploit.
Jim Meyering <jim@meyering.net> wrote on 03/05/2010 11:36:59:
From: Jim Meyering <jim@meyering.net> To: Kenneth Nagin/Haifa/IBM@IBMIL Cc: Cole Robinson <crobinso@redhat.com>, list libvirt <libvir-list@redhat.com> Date: 03/05/2010 11:37 Subject: Re: [libvirt] make syntax-check: [sc_vulnerable_makefile_CVE-2009-4029] Error 1
Cole Robinson <crobinso@redhat.com> wrote on 30/04/2010 15:42:05:
From: Cole Robinson <crobinso@redhat.com> To: Kenneth Nagin/Haifa/IBM@IBMIL Cc: "Daniel P. Berrange" <berrange@redhat.com>, list libvirt <libvir-list@redhat.com>, Daniel Veillard <veillard@redhat.com> Date: 30/04/2010 15:42 Subject: Re: [libvirt] (Resend) Live Migration with non-shared storage for kvm
Applying the patch (to last weeks checkout), there are some compilerwarnings: make sure you configure with --enable-compiler-warnings=error. 'make syntax-check' also fails, so please address these.
But I get this error message when compiling with 'make syntax-check':
2.14 unmarked_diagnostics vulnerable_makefile_CVE-2009-4029 ./Makefile.in:1283: -find $(distdir) -type d ! -perm -777 -exec chmod a+rwx {} \; -o \ maint.mk: the above files are vulnerable; beware of running "make dist*" rules, and upgrade to fixed automake see http://bugzilla.redhat.com/542609 for details make: *** [sc_vulnerable_makefile_CVE-2009-4029] Error 1
This problem is unrelated to any changes that I made and appearantly
Kenneth Nagin wrote: the
compile completes because make install works properly.
Any suggestions on how to resolve this error message.
That means you are using a version of automake that lacks the fix for the referenced bug. Upgrading to a patched version of automake, and regenerating all Makefile.in files will fix it.
If you run any make rule that runs that find command, you may expose yourself to a nasty exploit. I upgrade automake, but now make syntac-check fails with because LIBTOOL is undefined. I followed the instructions in the error message but it didn't help.
[root@hoover libvirt]# make syntax-check cd . && /bin/sh /gpfs/reservoir/nagin/workspace/libvirt_tip/libvirt/build-aux/missing --run automake-1.10 --gnu gnulib/lib/Makefile.am:21: Libtool library used but `LIBTOOL' is undefined gnulib/lib/Makefile.am:21: The usual way to define `LIBTOOL' is to add `AC_PROG_LIBTOOL' gnulib/lib/Makefile.am:21: to `configure.ac' and run `aclocal' and `autoconf' again. gnulib/lib/Makefile.am:21: If `AC_PROG_LIBTOOL' is in `configure.ac', make sure gnulib/lib/Makefile.am:21: its definition is in aclocal's search path. regards Kenneth Nagin
Kenneth Nagin wrote:
I upgrade automake, but now make syntac-check fails with because LIBTOOL is undefined. I followed the instructions in the error message but it didn't help.
[root@hoover libvirt]# make syntax-check cd . && /bin/sh /gpfs/reservoir/nagin/workspace/libvirt_tip/libvirt/build-aux/missing --run automake-1.10 --gnu gnulib/lib/Makefile.am:21: Libtool library used but `LIBTOOL' is undefined gnulib/lib/Makefile.am:21: The usual way to define `LIBTOOL' is to add `AC_PROG_LIBTOOL'
This is independent of "syntax-check". I suspect you should run "make distclean" or use some other method (e.g., like starting from a fresh clone) to ensure that you've regenerated everything required. Another possibility is that you built this new version of automake yourself and installed it early in your path. In that case, it is not using the same internal search path, which causes trouble unless you've also built libtool with the same $(prefix). In any case, I would recommend your using automake-1.11.1 or newer for all development.
gnulib/lib/Makefile.am:21: to `configure.ac' and run `aclocal' and `autoconf' again. gnulib/lib/Makefile.am:21: If `AC_PROG_LIBTOOL' is in `configure.ac', make sure gnulib/lib/Makefile.am:21: its definition is in aclocal's search path.
Cole Robinson <crobinso@redhat.com> wrote on 30/04/2010 15:42:05:
From: Cole Robinson <crobinso@redhat.com> To: Kenneth Nagin/Haifa/IBM@IBMIL Cc: "Daniel P. Berrange" <berrange@redhat.com>, list libvirt <libvir-list@redhat.com>, Daniel Veillard <veillard@redhat.com> Date: 30/04/2010 15:42 Subject: Re: [libvirt] (Resend) Live Migration with non-shared storage for kvm
Finding a way to post the patch in-line will also probably get better attention: just pasting it into the mail client will probably mangle the patch, I'd recommend git send-email.
- Cole
I'm new to git so I suspect that I don't understand the proper method for patch submission. But this is the problem that I see with your suggestion. git send-email implies usage of git-format-patch. But git-format-patch creates a set of files one per commit. However, don't you want to submit the diff between the changed code and the master, i.e. git diff master > patch? regards, Kenneth Nagin
Kenneth Nagin wrote:
Cole Robinson <crobinso@redhat.com> wrote on 30/04/2010 15:42:05: From: Cole Robinson <crobinso@redhat.com> To: Kenneth Nagin/Haifa/IBM@IBMIL Cc: "Daniel P. Berrange" <berrange@redhat.com>, list libvirt <libvir-list@redhat.com>, Daniel Veillard <veillard@redhat.com> Date: 30/04/2010 15:42 Subject: Re: [libvirt] (Resend) Live Migration with non-shared storage for kvm
Finding a way to post the patch in-line will also probably get better attention: just pasting it into the mail client will probably mangle the patch, I'd recommend git send-email.
I'm new to git so I suspect that I don't understand the proper method for patch submission. But this is the problem that I see with your suggestion. git send-email implies usage of git-format-patch. But git-format-patch creates a set of files one per commit.
If (and this is a big "if") your mail client does not split lines or mangle white space, it's ok to use e.g., git format-patch --stdout -1 > FILE and then to insert FILE in the body of your message. That will put the single, most recent change-set in FILE. If you want to submit a series of N (say N=5) change sets, you can s/-1/-5/: git format-patch --stdout -5 > FILE still assuming that the 5 most recent change sets are the ones you want. Regarding your mail client, you should try it first by sending yourself a patch. When you receive it, save the patch to a file, say PATCH, removing any ">" that were prepended to "From" lines, and then run "git checkout master; git am PATCH" and ensure that that succeeds. If there is any problem, go back to square 1 and either fix your client or use git send-email... Using "git send-email" is similar, but makes it easier for the recipient/reviewer to apply your patch.
However, don't you want to submit the diff between the changed code and the master, i.e. git diff master > patch?
Yes. Typically I copy the 00* patch files into an empty directory, cd into that directory and then run this: git send-email --compose --to=libvir-list@redhat.com 00* Be sure that you've set this in your ~/.gitconfig: git config --global sendemail.chainreplyto no
On 05/03/2010 04:41 AM, Kenneth Nagin wrote:
Cole Robinson <crobinso@redhat.com> wrote on 30/04/2010 15:42:05:
From: Cole Robinson <crobinso@redhat.com> To: Kenneth Nagin/Haifa/IBM@IBMIL Cc: "Daniel P. Berrange" <berrange@redhat.com>, list libvirt <libvir-list@redhat.com>, Daniel Veillard <veillard@redhat.com> Date: 30/04/2010 15:42 Subject: Re: [libvirt] (Resend) Live Migration with non-shared storage for kvm
Finding a way to post the patch in-line will also probably get better attention: just pasting it into the mail client will probably mangle the patch, I'd recommend git send-email.
- Cole
I'm new to git so I suspect that I don't understand the proper method for patch submission. But this is the problem that I see with your suggestion. git send-email implies usage of git-format-patch. But git-format-patch creates a set of files one per commit.
You can pass options to format-patch which will limit the range of commits it will dump: git format-patch -3 will dump the latest 3 commits for example. If your changes are spread over multiple commits and you want to submit it all as a single change, use git rebase -i to squash commits together.
However, don't you want to submit the diff between the changed code and the master, i.e. git diff master > patch?
A simplification of my workflow is: git checkout master git pull git checkout -b workbranch Hack, committing any changes along the way git rebase -i to clean up the commits git format-patch -# git send-email - Cole
On 05/03/2010 09:39 AM, Cole Robinson wrote:
A simplification of my workflow is:
git checkout master git pull git checkout -b workbranch Hack, committing any changes along the way git rebase -i to clean up the commits
The one thing I would add here would be: git checkout master git pull git checkout workbranch git rebase master (fix any conflicts) to bring your workbranch up to date with master, and make it less likely others would encounter conflicts when they try to apply your patches to their work tree.
git format-patch -# git send-email
I recently learned that git send-email will send directly from the git history, without the bother of using git format-patch to create intermediate files. For example: git send-email -1 will send an email containing the latest commit on the current branch. git send-email --compose -5 will open your editor of choice to compose an email introducing the patch series (you should put "[PATCH 0/n]" at the beginning of the subject where n is, in this example, 5). After you've saved that mail, it will send the introduction mail followed by a single email for each of the last 5 commits on the current branch. I much prefer doing it this way, because I don't end up with tons of patchfiles lying around (which usually results in me sending the wrong set of files).
On 05/03/2010 08:26 AM, Laine Stump wrote:
I recently learned that git send-email will send directly from the git history, without the bother of using git format-patch to create intermediate files. For example:
git send-email -1
will send an email containing the latest commit on the current branch.
git send-email --compose -5
Another useful command line: git send-email -5 --in-reply-to=... \ --annotate --compose --subject-prefix=PATCHv2 which lets me send a 5-patch series as a v2 reply to an earlier email, with a cover letter, and allowing me to add comments to each individual patch to document what changed from v1. The only thing that makes this hard is that thunderbird 2.x used to have the mnenhy plugin to make it very easy to get the Message-ID of any given message, but that plugin has not been ported to thunderbird 3.0 so I have to view the source of a message and manually find the MessageID. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
On 05/03/2010 11:13 AM, Eric Blake wrote:
On 05/03/2010 08:26 AM, Laine Stump wrote:
I recently learned that git send-email will send directly from the git history, without the bother of using git format-patch to create intermediate files. For example:
git send-email -1
will send an email containing the latest commit on the current branch.
git send-email --compose -5
Another useful command line:
git send-email -5 --in-reply-to=... \ --annotate --compose --subject-prefix=PATCHv2
which lets me send a 5-patch series as a v2 reply to an earlier email, with a cover letter, and allowing me to add comments to each individual patch to document what changed from v1. The only thing that makes this hard is that thunderbird 2.x used to have the mnenhy plugin to make it very easy to get the Message-ID of any given message, but that plugin has not been ported to thunderbird 3.0 so I have to view the source of a message and manually find the MessageID.
I just select View/Headers/All, then right click on the "1" next to the "message-id" line, and select "Copy". (I wish I could select that, and a couple other extra headers, to be displayed when "Normal" headers are selected. With all the time I spend using Thunderbird every day, you'd think I'd take the time to figure out where/how to make feature requests :-P)
participants (6)
-
Cole Robinson -
Daniel P. Berrange -
Eric Blake -
Jim Meyering -
Kenneth Nagin -
Laine Stump