[libvirt] [PATCH v2 0/4] 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 unknown 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... Version 2: - add virStorageFileIsClusterFS as suggested by Dan B. Jiri Denemark (4): Add support for unsafe migration virsh: Add --unsafe option to migrate command Introduce virStorageFileIsClusterFS qemu: Forbid migration with cache != none include/libvirt/libvirt.h.in | 2 +- include/libvirt/virterror.h | 1 + src/libvirt.c | 4 ++++ src/libvirt_private.syms | 1 + src/qemu/qemu_driver.c | 3 ++- src/qemu/qemu_migration.c | 39 +++++++++++++++++++++++++++++++++++---- src/qemu/qemu_migration.h | 6 ++++-- src/util/storage_file.c | 10 ++++++++++ src/util/storage_file.h | 1 + src/util/virterror.c | 6 ++++++ tools/virsh.c | 4 ++++ tools/virsh.pod | 10 +++++++++- 12 files changed, 78 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. --- Notes: Version 2: - no change 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 6294196..a3bd4f4 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

On 02/22/2012 07:51 AM, Jiri Denemark wrote:
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.
ACK. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org

--- Notes: Version 2: - no change 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 e712e53..3be86ed 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -6819,6 +6819,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)")}, @@ -6892,6 +6893,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 f35244f..0f1ea91 100644 --- a/tools/virsh.pod +++ b/tools/virsh.pod @@ -781,7 +781,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>] @@ -802,6 +802,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

On 02/22/2012 07:51 AM, Jiri Denemark wrote:
--- Notes: Version 2: - no change
tools/virsh.c | 4 ++++ tools/virsh.pod | 10 +++++++++- 2 files changed, 13 insertions(+), 1 deletions(-)
@@ -802,6 +802,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.
That doesn't flow very well. How about: because doing so may lead to potential problems such as 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.
ACK with the wording tweak. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org

--- Notes: Version 2: - new patch src/libvirt_private.syms | 1 + src/util/storage_file.c | 10 ++++++++++ src/util/storage_file.h | 1 + 3 files changed, 12 insertions(+), 0 deletions(-) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 9e3573f..18a24e4 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1011,6 +1011,7 @@ virStorageFileFormatTypeToString; virStorageFileFreeMetadata; virStorageFileGetMetadata; virStorageFileGetMetadataFromFD; +virStorageFileIsClusterFS; virStorageFileIsSharedFS; virStorageFileIsSharedFSType; virStorageFileProbeFormat; diff --git a/src/util/storage_file.c b/src/util/storage_file.c index a8661e3..cd1c142 100644 --- a/src/util/storage_file.c +++ b/src/util/storage_file.c @@ -1063,3 +1063,13 @@ int virStorageFileIsSharedFS(const char *path) VIR_STORAGE_FILE_SHFS_OCFS | VIR_STORAGE_FILE_SHFS_AFS); } + +int virStorageFileIsClusterFS(const char *path) +{ + /* These are coherent cluster filesystems known to be safe for + * migration with cache != none + */ + return virStorageFileIsSharedFSType(path, + VIR_STORAGE_FILE_SHFS_GFS2 | + VIR_STORAGE_FILE_SHFS_OCFS); +} diff --git a/src/util/storage_file.h b/src/util/storage_file.h index 96afb12..13d0e87 100644 --- a/src/util/storage_file.h +++ b/src/util/storage_file.h @@ -82,6 +82,7 @@ enum { }; int virStorageFileIsSharedFS(const char *path); +int virStorageFileIsClusterFS(const char *path); int virStorageFileIsSharedFSType(const char *path, int fstypes); -- 1.7.8.4

On 02/22/2012 07:51 AM, Jiri Denemark wrote:
--- Notes: Version 2: - new patch
ACK. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org

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. --- Notes: Version 2: - use virStorageFileIsClusterFS src/qemu/qemu_driver.c | 3 ++- src/qemu/qemu_migration.c | 39 +++++++++++++++++++++++++++++++++++---- src/qemu/qemu_migration.h | 6 ++++-- 3 files changed, 41 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..09494d6 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -45,6 +45,7 @@ #include "virtime.h" #include "locking/domain_lock.h" #include "rpc/virnetsocket.h" +#include "storage_file.h" #define VIR_FROM_THIS VIR_FROM_QEMU @@ -817,6 +818,29 @@ 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->src && + disk->cachemode != VIR_DOMAIN_DISK_CACHE_DISABLE && + (disk->cachemode || !disk->shared || disk->readonly) && + virStorageFileIsClusterFS(disk->src) == 1) { + 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 +1034,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 +1043,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 +1057,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 +2098,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 +2382,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 Wed, Feb 22, 2012 at 03:51:11PM +0100, 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. --- Notes: Version 2: - use virStorageFileIsClusterFS
src/qemu/qemu_driver.c | 3 ++- src/qemu/qemu_migration.c | 39 +++++++++++++++++++++++++++++++++++---- src/qemu/qemu_migration.h | 6 ++++-- 3 files changed, 41 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..09494d6 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -45,6 +45,7 @@ #include "virtime.h" #include "locking/domain_lock.h" #include "rpc/virnetsocket.h" +#include "storage_file.h"
#define VIR_FROM_THIS VIR_FROM_QEMU @@ -817,6 +818,29 @@ 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->src && + disk->cachemode != VIR_DOMAIN_DISK_CACHE_DISABLE && + (disk->cachemode || !disk->shared || disk->readonly) && + virStorageFileIsClusterFS(disk->src) == 1) {
Isn't this test reversed. ie, we want to deny migration if *not* a cluster FS, eg == 0 surely ? 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 :|

On Wed, Feb 22, 2012 at 14:57:00 +0000, Daniel P. Berrange wrote:
On Wed, Feb 22, 2012 at 03:51:11PM +0100, 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. --- Notes: Version 2: - use virStorageFileIsClusterFS
src/qemu/qemu_driver.c | 3 ++- src/qemu/qemu_migration.c | 39 +++++++++++++++++++++++++++++++++++---- src/qemu/qemu_migration.h | 6 ++++-- 3 files changed, 41 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..09494d6 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -45,6 +45,7 @@ #include "virtime.h" #include "locking/domain_lock.h" #include "rpc/virnetsocket.h" +#include "storage_file.h"
#define VIR_FROM_THIS VIR_FROM_QEMU @@ -817,6 +818,29 @@ 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->src && + disk->cachemode != VIR_DOMAIN_DISK_CACHE_DISABLE && + (disk->cachemode || !disk->shared || disk->readonly) && + virStorageFileIsClusterFS(disk->src) == 1) {
Isn't this test reversed. ie, we want to deny migration if *not* a cluster FS, eg == 0 surely ?
Eh, sure. Thanks. Jirka

On 02/22/2012 07:51 AM, 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. --- Notes: Version 2: - use virStorageFileIsClusterFS
src/qemu/qemu_driver.c | 3 ++- src/qemu/qemu_migration.c | 39 +++++++++++++++++++++++++++++++++++---- src/qemu/qemu_migration.h | 6 ++++-- 3 files changed, 41 insertions(+), 7 deletions(-)
+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->src && + disk->cachemode != VIR_DOMAIN_DISK_CACHE_DISABLE && + (disk->cachemode || !disk->shared || disk->readonly) && + virStorageFileIsClusterFS(disk->src) == 1) {
Other than Dan's comment about the logic bug here, ACK. Since the check for safety is only on the source, and the destination doesn't care, is there a way to add a driver feature flag, and add logic to libvirt.c to mask the VIR_MIGRATE_UNSAFE flag from the destination if it does not support the feature, similar to how we handled VIR_MIGRATE_CHANGE_PROTECTION via the VIR_DRV_FEATURE_MIGRATE_CHANGE_PROTECTION feature, as another example of a source-only flag? Of course, this would be a followup patch, if we decide it is worth allowing an unsafe migration from 1.9.11 back to 1.9.10 (the upgrade migration from 1.9.10 to 1.9.11 will be unsafe automatically, because we weren't checking in 1.9.10). -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org

On Wed, Feb 22, 2012 at 12:54:35 -0700, Eric Blake wrote:
On 02/22/2012 07:51 AM, 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. --- Notes: Version 2: - use virStorageFileIsClusterFS
src/qemu/qemu_driver.c | 3 ++- src/qemu/qemu_migration.c | 39 +++++++++++++++++++++++++++++++++++---- src/qemu/qemu_migration.h | 6 ++++-- 3 files changed, 41 insertions(+), 7 deletions(-)
+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->src && + disk->cachemode != VIR_DOMAIN_DISK_CACHE_DISABLE && + (disk->cachemode || !disk->shared || disk->readonly) && + virStorageFileIsClusterFS(disk->src) == 1) {
Other than Dan's comment about the logic bug here, ACK.
Actually, I rewrote this as + if (disk->src && + disk->cachemode != VIR_DOMAIN_DISK_CACHE_DISABLE && + (disk->cachemode || !disk->shared || disk->readonly)) { + int cfs; + if ((cfs = virStorageFileIsClusterFS(disk->src)) == 1) + continue; + else if (cfs < 0) + return false; + + qemuReportError(VIR_ERR_MIGRATE_UNSAFE, "%s", + _("Migration may lead to data corruption if disks" + " use cache != none")); + return false; + } to avoid overwriting errors returned by virStorageFileIsClusterFS().
Since the check for safety is only on the source, and the destination doesn't care, is there a way to add a driver feature flag, and add logic to libvirt.c to mask the VIR_MIGRATE_UNSAFE flag from the destination if it does not support the feature, similar to how we handled VIR_MIGRATE_CHANGE_PROTECTION via the VIR_DRV_FEATURE_MIGRATE_CHANGE_PROTECTION feature, as another example of a source-only flag? Of course, this would be a followup patch, if we decide it is worth allowing an unsafe migration from 1.9.11 back to 1.9.10 (the upgrade migration from 1.9.10 to 1.9.11 will be unsafe automatically, because we weren't checking in 1.9.10).
Yeah, it would be possible to do it, however as we already added at least new qemu capability (system_wakeup), domains running on new enough qemu will not be migratable to 0.9.10 anyway so I guess it's just not worth it :-) And I pushed this series, thanks for the reviews. Jirka

On 02/22/2012 07:51 AM, Jiri Denemark wrote:
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...
I think that if we ever have sub-categories of unsafe operations, where we want the user to pass varying flags to allow one but not the other sub-category, then we could do: VIR_ERR_MIGRATION_UNSAFE_CACHE = 1<<9, VIR_ERR_MIGRATION_UNSAFE_DISK = 1<<10, VIR_ERR_MIGRATION_UNSAFE = (VIR_ERR_MIGRATION_UNSAFE_CACHE|VIR_ERR_MIGRATION_UNSAFE_DISK) that is, make the current generic name remain generic by having it cover multiple bits, while the specific bits control the sub-options. But for now, I'm fine with just a single, shorter, name. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
participants (3)
-
Daniel P. Berrange
-
Eric Blake
-
Jiri Denemark