[libvirt] [PATCH 0/3] Forbid migration with cache != none
Migrating qemu domains with disks using cache != none is unsafe unless the disk images are stored on coherent clustered filesystem. Thus we forbid migrating such domains unless VIR_MIGRATE_UNSAFE flags is used. This series uses similar aproach to forbidding unsafe PCI passthrough or disk format probing when we forbade those by default with the possibility to force them. Domain configuration is only checked on source, which makes migrating affected domains from an old libvirt to the new one possible. Migrating back is impossible since destination libvirtd would complain about unknown flag (the flag is not filtered so it gets to the destination even though it's not really used there). However, users of clustered filesystems now have to always pass the new flag to be able to migrate because libvirtd would think they are doing something unsafe. Perhaps we should provide a system wide (i.e., /etc/libvirt/qemu.conf) tunable which would disable cache mode checking for all domains at once? I was also wondering if we should rather use more specific name for both the error code and flag, such as VIR(_ERR)?_MIGRATE_UNSAFE_CACHE (or ...UNSAFE_DISK) in the case we find other unsafe conditions... Jiri Denemark (3): Add support for unsafe migration virsh: Add --unsafe option to migrate command qemu: Forbid migration with cache != none include/libvirt/libvirt.h.in | 2 +- include/libvirt/virterror.h | 1 + src/libvirt.c | 4 ++++ src/qemu/qemu_driver.c | 3 ++- src/qemu/qemu_migration.c | 36 ++++++++++++++++++++++++++++++++---- src/qemu/qemu_migration.h | 6 ++++-- src/util/virterror.c | 6 ++++++ tools/virsh.c | 4 ++++ tools/virsh.pod | 10 +++++++++- 9 files changed, 63 insertions(+), 9 deletions(-) -- 1.7.8.4
This patch adds VIR_MIGRATE_UNSAFE flag for migration APIs and new VIR_ERR_MIGRATION_UNSAFE error code. The error code should be returned whenever migrating a domain is considered unsafe (e.g., it's configured in a way that does not ensure data integrity once it is migrated). VIR_MIGRATE_UNSAFE flag may be used to force migration even though it would normally be considered unsafe and forbidden. --- include/libvirt/libvirt.h.in | 2 +- include/libvirt/virterror.h | 1 + src/libvirt.c | 4 ++++ src/util/virterror.c | 6 ++++++ 4 files changed, 12 insertions(+), 1 deletions(-) diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in index 798ab07..e29df2a 100644 --- a/include/libvirt/libvirt.h.in +++ b/include/libvirt/libvirt.h.in @@ -934,7 +934,7 @@ typedef enum { VIR_MIGRATE_CHANGE_PROTECTION = (1 << 8), /* protect for changing domain configuration through the * whole migration process; this will be used automatically * when supported */ - + VIR_MIGRATE_UNSAFE = (1 << 9), /* force migration even if it is considered unsafe */ } virDomainMigrateFlags; /* Domain migration. */ diff --git a/include/libvirt/virterror.h b/include/libvirt/virterror.h index 9dbadfe..a3f9199 100644 --- a/include/libvirt/virterror.h +++ b/include/libvirt/virterror.h @@ -245,6 +245,7 @@ typedef enum { canceled/aborted by user */ VIR_ERR_AUTH_CANCELLED = 79, /* authentication cancelled */ VIR_ERR_NO_DOMAIN_METADATA = 80, /* The metadata is not present */ + VIR_ERR_MIGRATE_UNSAFE = 81, /* Migration is not safe */ } virErrorNumber; /** diff --git a/src/libvirt.c b/src/libvirt.c index a55d823..17c4d15 100644 --- a/src/libvirt.c +++ b/src/libvirt.c @@ -5110,6 +5110,7 @@ virDomainMigrateDirect (virDomainPtr domain, * VIR_MIGRATE_CHANGE_PROTECTION Protect against domain configuration * changes during the migration process (set * automatically when supported). + * VIR_MIGRATE_UNSAFE Force migration even if it is considered unsafe. * * VIR_MIGRATE_TUNNELLED requires that VIR_MIGRATE_PEER2PEER be set. * Applications using the VIR_MIGRATE_PEER2PEER flag will probably @@ -5301,6 +5302,7 @@ error: * VIR_MIGRATE_CHANGE_PROTECTION Protect against domain configuration * changes during the migration process (set * automatically when supported). + * VIR_MIGRATE_UNSAFE Force migration even if it is considered unsafe. * * VIR_MIGRATE_TUNNELLED requires that VIR_MIGRATE_PEER2PEER be set. * Applications using the VIR_MIGRATE_PEER2PEER flag will probably @@ -5509,6 +5511,7 @@ error: * VIR_MIGRATE_CHANGE_PROTECTION Protect against domain configuration * changes during the migration process (set * automatically when supported). + * VIR_MIGRATE_UNSAFE Force migration even if it is considered unsafe. * * The operation of this API hinges on the VIR_MIGRATE_PEER2PEER flag. * If the VIR_MIGRATE_PEER2PEER flag is NOT set, the duri parameter @@ -5633,6 +5636,7 @@ error: * VIR_MIGRATE_CHANGE_PROTECTION Protect against domain configuration * changes during the migration process (set * automatically when supported). + * VIR_MIGRATE_UNSAFE Force migration even if it is considered unsafe. * * The operation of this API hinges on the VIR_MIGRATE_PEER2PEER flag. * diff --git a/src/util/virterror.c b/src/util/virterror.c index fb5ca6f..de60185 100644 --- a/src/util/virterror.c +++ b/src/util/virterror.c @@ -1232,6 +1232,12 @@ virErrorMsg(virErrorNumber error, const char *info) else errmsg = _("metadata not found: %s"); break; + case VIR_ERR_MIGRATE_UNSAFE: + if (!info) + errmsg = _("Unsafe migration"); + else + errmsg = _("Unsafe migration: %s"); + break; } return (errmsg); } -- 1.7.8.4
--- tools/virsh.c | 4 ++++ tools/virsh.pod | 10 +++++++++- 2 files changed, 13 insertions(+), 1 deletions(-) diff --git a/tools/virsh.c b/tools/virsh.c index 66ba61c..4e30325 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -6733,6 +6733,7 @@ static const vshCmdOptDef opts_migrate[] = { {"copy-storage-inc", VSH_OT_BOOL, 0, N_("migration with non-shared storage with incremental copy (same base image shared between source and destination)")}, {"change-protection", VSH_OT_BOOL, 0, N_("prevent any configuration changes to domain until migration ends)")}, + {"unsafe", VSH_OT_BOOL, 0, N_("force migration even if it may be unsafe")}, {"verbose", VSH_OT_BOOL, 0, N_("display the progress of migration")}, {"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 as seen from the client(normal migration) or source(p2p migration)")}, @@ -6806,6 +6807,9 @@ doMigrate (void *opaque) if (vshCommandOptBool (cmd, "change-protection")) flags |= VIR_MIGRATE_CHANGE_PROTECTION; + if (vshCommandOptBool(cmd, "unsafe")) + flags |= VIR_MIGRATE_UNSAFE; + if (xmlfile && virFileReadAll(xmlfile, 8192, &xml) < 0) { vshError(ctl, _("file '%s' doesn't exist"), xmlfile); diff --git a/tools/virsh.pod b/tools/virsh.pod index 0d5a41d..c9c7b13 100644 --- a/tools/virsh.pod +++ b/tools/virsh.pod @@ -765,7 +765,7 @@ type attribute for the <domain> element of XML. =item B<migrate> [I<--live>] [I<--direct>] [I<--p2p> [I<--tunnelled>]] [I<--persistent>] [I<--undefinesource>] [I<--suspend>] [I<--copy-storage-all>] -[I<--copy-storage-inc>] [I<--change-protection>] [I<--verbose>] +[I<--copy-storage-inc>] [I<--change-protection>] [I<--unsafe>] [I<--verbose>] I<domain-id> I<desturi> [I<migrateuri>] [I<dname>] [I<--timeout> B<seconds>] [I<--xml> B<file>] @@ -786,6 +786,14 @@ is implicitly enabled when supported by the hypervisor, but can be explicitly used to reject the migration if the hypervisor lacks change protection support. I<--verbose> displays the progress of migration. +In some cases libvirt may refuse to migrate the domain because doing so may +lead, e.g., to data corruption and thus the migration is considered unsafe. +For QEMU domain, this may happen if the domain uses disks without explicitly +setting cache mode to "none". Migrating such domains is unsafe unless the disk +images are stored on coherent clustered filesystem, such as GFS2 or GPFS. If +you are sure the migration is safe or you just do not care, use I<--unsafe> +to force the migration. + The I<desturi> is the connection URI of the destination host, and I<migrateuri> is the migration URI, which usually can be omitted. I<dname> is used for renaming the domain to new name during migration, which -- 1.7.8.4
Migrating domains with disks using cache != none is unsafe unless the disk images are stored on coherent clustered filesystem. Thus we forbid migrating such domains unless VIR_MIGRATE_UNSAFE flags is used. --- src/qemu/qemu_driver.c | 3 ++- src/qemu/qemu_migration.c | 36 ++++++++++++++++++++++++++++++++---- src/qemu/qemu_migration.h | 6 ++++-- 3 files changed, 38 insertions(+), 7 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 717bdf1..63a0703 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -8767,7 +8767,8 @@ qemuDomainMigrateBegin3(virDomainPtr domain, goto endjob; if (!(xml = qemuMigrationBegin(driver, vm, xmlin, dname, - cookieout, cookieoutlen))) + cookieout, cookieoutlen, + flags))) goto endjob; if ((flags & VIR_MIGRATE_CHANGE_PROTECTION)) { diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index f0af494..127b35c 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -817,6 +817,27 @@ qemuMigrationIsAllowed(struct qemud_driver *driver, virDomainObjPtr vm, return true; } +static bool +qemuMigrationIsSafe(virDomainDefPtr def) +{ + int i; + + for (i = 0 ; i < def->ndisks ; i++) { + virDomainDiskDefPtr disk = def->disks[i]; + + /* shared && !readonly implies cache=none */ + if (disk->cachemode != VIR_DOMAIN_DISK_CACHE_DISABLE && + (disk->cachemode || !disk->shared || disk->readonly)) { + qemuReportError(VIR_ERR_MIGRATE_UNSAFE, "%s", + _("Migration may lead to data corruption if disks" + " use cache != none")); + return false; + } + } + + return true; +} + /** qemuMigrationSetOffline * Pause domain for non-live migration. */ @@ -1010,7 +1031,8 @@ char *qemuMigrationBegin(struct qemud_driver *driver, const char *xmlin, const char *dname, char **cookieout, - int *cookieoutlen) + int *cookieoutlen, + unsigned long flags) { char *rv = NULL; qemuMigrationCookiePtr mig = NULL; @@ -1018,9 +1040,9 @@ char *qemuMigrationBegin(struct qemud_driver *driver, qemuDomainObjPrivatePtr priv = vm->privateData; VIR_DEBUG("driver=%p, vm=%p, xmlin=%s, dname=%s," - " cookieout=%p, cookieoutlen=%p", + " cookieout=%p, cookieoutlen=%p, flags=%lx", driver, vm, NULLSTR(xmlin), NULLSTR(dname), - cookieout, cookieoutlen); + cookieout, cookieoutlen, flags); /* Only set the phase if we are inside QEMU_ASYNC_JOB_MIGRATION_OUT. * Otherwise we will start the async job later in the perform phase losing @@ -1032,6 +1054,9 @@ char *qemuMigrationBegin(struct qemud_driver *driver, if (!qemuMigrationIsAllowed(driver, vm, NULL)) goto cleanup; + if (!(flags & VIR_MIGRATE_UNSAFE) && !qemuMigrationIsSafe(vm->def)) + goto cleanup; + if (!(mig = qemuMigrationEatCookie(driver, vm, NULL, 0, 0))) goto cleanup; @@ -2070,7 +2095,7 @@ static int doPeer2PeerMigrate3(struct qemud_driver *driver, * a single job. */ dom_xml = qemuMigrationBegin(driver, vm, xmlin, dname, - &cookieout, &cookieoutlen); + &cookieout, &cookieoutlen, flags); if (!dom_xml) goto cleanup; @@ -2354,6 +2379,9 @@ qemuMigrationPerformJob(struct qemud_driver *driver, if (!qemuMigrationIsAllowed(driver, vm, NULL)) goto cleanup; + if (!(flags & VIR_MIGRATE_UNSAFE) && !qemuMigrationIsSafe(vm->def)) + goto cleanup; + resume = virDomainObjGetState(vm, NULL) == VIR_DOMAIN_RUNNING; if ((flags & (VIR_MIGRATE_TUNNELLED | VIR_MIGRATE_PEER2PEER))) { diff --git a/src/qemu/qemu_migration.h b/src/qemu/qemu_migration.h index f806ca1..41e4eac 100644 --- a/src/qemu/qemu_migration.h +++ b/src/qemu/qemu_migration.h @@ -35,7 +35,8 @@ VIR_MIGRATE_PAUSED | \ VIR_MIGRATE_NON_SHARED_DISK | \ VIR_MIGRATE_NON_SHARED_INC | \ - VIR_MIGRATE_CHANGE_PROTECTION) + VIR_MIGRATE_CHANGE_PROTECTION | \ + VIR_MIGRATE_UNSAFE) enum qemuMigrationJobPhase { QEMU_MIGRATION_PHASE_NONE = 0, @@ -81,7 +82,8 @@ char *qemuMigrationBegin(struct qemud_driver *driver, const char *xmlin, const char *dname, char **cookieout, - int *cookieoutlen); + int *cookieoutlen, + unsigned long flags); int qemuMigrationPrepareTunnel(struct qemud_driver *driver, virConnectPtr dconn, -- 1.7.8.4
On 2012-2-22 0:17, Jiri Denemark wrote:
Migrating domains with disks using cache != none is unsafe unless the disk images are stored on coherent clustered filesystem. Thus we forbid migrating such domains unless VIR_MIGRATE_UNSAFE flags is used. --- src/qemu/qemu_driver.c | 3 ++- src/qemu/qemu_migration.c | 36 ++++++++++++++++++++++++++++++++---- src/qemu/qemu_migration.h | 6 ++++-- 3 files changed, 38 insertions(+), 7 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 717bdf1..63a0703 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -8767,7 +8767,8 @@ qemuDomainMigrateBegin3(virDomainPtr domain, goto endjob;
if (!(xml = qemuMigrationBegin(driver, vm, xmlin, dname, - cookieout, cookieoutlen))) + cookieout, cookieoutlen, + flags))) goto endjob;
if ((flags& VIR_MIGRATE_CHANGE_PROTECTION)) { diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index f0af494..127b35c 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -817,6 +817,27 @@ qemuMigrationIsAllowed(struct qemud_driver *driver, virDomainObjPtr vm, return true; }
+static bool +qemuMigrationIsSafe(virDomainDefPtr def) +{ + int i; + + for (i = 0 ; i< def->ndisks ; i++) { + virDomainDiskDefPtr disk = def->disks[i]; + + /* shared&& !readonly implies cache=none */ + if (disk->cachemode != VIR_DOMAIN_DISK_CACHE_DISABLE&&
how about disk write through flag here? This flag should also imply cache=none. i.e, VIR_DOMAIN_DISK_CACHE_WRITETHRU
+ (disk->cachemode || !disk->shared || disk->readonly)) { + qemuReportError(VIR_ERR_MIGRATE_UNSAFE, "%s", + _("Migration may lead to data corruption if disks" + " use cache != none")); + return false; + } + } + + return true; +} + /** qemuMigrationSetOffline * Pause domain for non-live migration. */ @@ -1010,7 +1031,8 @@ char *qemuMigrationBegin(struct qemud_driver *driver, const char *xmlin, const char *dname, char **cookieout, - int *cookieoutlen) + int *cookieoutlen, + unsigned long flags) { char *rv = NULL; qemuMigrationCookiePtr mig = NULL; @@ -1018,9 +1040,9 @@ char *qemuMigrationBegin(struct qemud_driver *driver, qemuDomainObjPrivatePtr priv = vm->privateData;
VIR_DEBUG("driver=%p, vm=%p, xmlin=%s, dname=%s," - " cookieout=%p, cookieoutlen=%p", + " cookieout=%p, cookieoutlen=%p, flags=%lx", driver, vm, NULLSTR(xmlin), NULLSTR(dname), - cookieout, cookieoutlen); + cookieout, cookieoutlen, flags);
/* Only set the phase if we are inside QEMU_ASYNC_JOB_MIGRATION_OUT. * Otherwise we will start the async job later in the perform phase losing @@ -1032,6 +1054,9 @@ char *qemuMigrationBegin(struct qemud_driver *driver, if (!qemuMigrationIsAllowed(driver, vm, NULL)) goto cleanup;
+ if (!(flags& VIR_MIGRATE_UNSAFE)&& !qemuMigrationIsSafe(vm->def)) + goto cleanup; + if (!(mig = qemuMigrationEatCookie(driver, vm, NULL, 0, 0))) goto cleanup;
@@ -2070,7 +2095,7 @@ static int doPeer2PeerMigrate3(struct qemud_driver *driver, * a single job. */
dom_xml = qemuMigrationBegin(driver, vm, xmlin, dname, -&cookieout,&cookieoutlen); +&cookieout,&cookieoutlen, flags); if (!dom_xml) goto cleanup;
@@ -2354,6 +2379,9 @@ qemuMigrationPerformJob(struct qemud_driver *driver, if (!qemuMigrationIsAllowed(driver, vm, NULL)) goto cleanup;
+ if (!(flags& VIR_MIGRATE_UNSAFE)&& !qemuMigrationIsSafe(vm->def)) + goto cleanup; + resume = virDomainObjGetState(vm, NULL) == VIR_DOMAIN_RUNNING;
if ((flags& (VIR_MIGRATE_TUNNELLED | VIR_MIGRATE_PEER2PEER))) { diff --git a/src/qemu/qemu_migration.h b/src/qemu/qemu_migration.h index f806ca1..41e4eac 100644 --- a/src/qemu/qemu_migration.h +++ b/src/qemu/qemu_migration.h @@ -35,7 +35,8 @@ VIR_MIGRATE_PAUSED | \ VIR_MIGRATE_NON_SHARED_DISK | \ VIR_MIGRATE_NON_SHARED_INC | \ - VIR_MIGRATE_CHANGE_PROTECTION) + VIR_MIGRATE_CHANGE_PROTECTION | \ + VIR_MIGRATE_UNSAFE)
enum qemuMigrationJobPhase { QEMU_MIGRATION_PHASE_NONE = 0, @@ -81,7 +82,8 @@ char *qemuMigrationBegin(struct qemud_driver *driver, const char *xmlin, const char *dname, char **cookieout, - int *cookieoutlen); + int *cookieoutlen, + unsigned long flags);
int qemuMigrationPrepareTunnel(struct qemud_driver *driver, virConnectPtr dconn,
-- Shu Ming<shuming@linux.vnet.ibm.com> IBM China Systems and Technology Laboratory
On Wed, Feb 22, 2012 at 14:58:31 +0800, Shu Ming wrote:
On 2012-2-22 0:17, Jiri Denemark wrote:
@@ -817,6 +817,27 @@ qemuMigrationIsAllowed(struct qemud_driver *driver, virDomainObjPtr vm, return true; }
+static bool +qemuMigrationIsSafe(virDomainDefPtr def) +{ + int i; + + for (i = 0 ; i< def->ndisks ; i++) { + virDomainDiskDefPtr disk = def->disks[i]; + + /* shared&& !readonly implies cache=none */ + if (disk->cachemode != VIR_DOMAIN_DISK_CACHE_DISABLE&&
how about disk write through flag here? This flag should also imply cache=none.
i.e, VIR_DOMAIN_DISK_CACHE_WRITETHRU
AFAIK, only VIR_DOMAIN_DISK_CACHE_DISABLE translates into cache=none unless qemu binary is old enough to only support cache=on|off Jirka
On 02/22/2012 02:57 PM, Jiri Denemark wrote:
how about disk write through flag here? This flag should also imply cache=none.
i.e, VIR_DOMAIN_DISK_CACHE_WRITETHRU AFAIK, only VIR_DOMAIN_DISK_CACHE_DISABLE translates into cache=none unless qemu binary is old enough to only support cache=on|off
VIR_DOMAIN_DISK_CACHE_DIRECTSYNC too. Paolo
On Tue, Feb 21, 2012 at 05:17:20PM +0100, Jiri Denemark wrote:
Migrating qemu domains with disks using cache != none is unsafe unless the disk images are stored on coherent clustered filesystem. Thus we forbid migrating such domains unless VIR_MIGRATE_UNSAFE flags is used.
This series uses similar aproach to forbidding unsafe PCI passthrough or disk format probing when we forbade those by default with the possibility to force them.
Domain configuration is only checked on source, which makes migrating affected domains from an old libvirt to the new one possible. Migrating back is impossible since destination libvirtd would complain about unknown flag (the flag is not filtered so it gets to the destination even though it's not really used there).
However, users of clustered filesystems now have to always pass the new flag to be able to migrate because libvirtd would think they are doing something unsafe. Perhaps we should provide a system wide (i.e., /etc/libvirt/qemu.conf) tunable which would disable cache mode checking for all domains at once?
I think we should add a virStorageFileIsClusterFS(const char *path) and whitelist the filesystem magic that we know is ok. I expect GFS, GFS2, OCFS2 would be a good starting point. We already do this for virStorageFileIsSharedFS() in migration, so this is no worse Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
participants (4)
-
Daniel P. Berrange -
Jiri Denemark -
Paolo Bonzini -
Shu Ming