[libvirt] [PATCH 0/2] Allow to make disk optional on migration
If user wants to migrate a machine, it's all or nothing. Either he/she has all storage accessible under same paths, or migration will fail. But there are some cases, where allowing migration to drop some disks, may be useful. E.g. when host gets itself to a state where domains need to be migrated so host can be say rebooted. Therefore we might want to give user possibility to mark some cdroms/floppy as optional, meaning if destination cannot access them, they get ejected (on destination). This idea is implemented via <migration> element, which basically says what to do with (currently) disk on migration. Right now it contains only one attribute 'optional' accepting values 'yes' and 'no'. Then, if destination cannot access a path and corresponding disk is mared as: a) optional - it is ejected (source is free()'d) b) non-optional - migration simply fails. The default is understandably non-optional to all disks. NB, setting optional is supported only on cdrom & floppy. Michal Privoznik (2): migration: Introduce <migration> element for cdrom and floppy qemu: Implement migration optional disk docs/schemas/domaincommon.rng | 16 ++++++++ src/conf/domain_conf.c | 85 ++++++++++++++++++++++++++++++++++++++++- src/conf/domain_conf.h | 16 ++++++++ src/libvirt_private.syms | 2 + src/qemu/qemu_migration.c | 47 +++++++++++++++++++++- 5 files changed, 163 insertions(+), 3 deletions(-) -- 1.7.3.4
This element says what to do with cdrom (or floppy) on migration. Currently, only one attribute is supported: 'optional'. It accepts 'yes' and 'no' values. Setting a cdrom to be optional means, if destination cannot access disk source, it simply gets free()'d/ejected. This functionality is important for users, whose machines get buggy. So they decide to save as much as possible and migrate the machine even they won't be able to access (readonly) cdrom on destination. --- docs/schemas/domaincommon.rng | 16 ++++++++ src/conf/domain_conf.c | 85 ++++++++++++++++++++++++++++++++++++++++- src/conf/domain_conf.h | 16 ++++++++ src/libvirt_private.syms | 2 + 4 files changed, 118 insertions(+), 1 deletions(-) diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index be98be0..1150297 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -630,6 +630,9 @@ <ref name="encryption"/> </optional> <optional> + <ref name="migration"/> + </optional> + <optional> <ref name="address"/> </optional> </define> @@ -642,6 +645,19 @@ </choice> </attribute> </define> + <define name="migration"> + <optional> + <element name="migration"> + <attribute name="optional"> + <choice> + <value>yes</value> + <value>no</value> + </choice> + </attribute> + <empty/> + </element> + </optional> + </define> <define name="lease"> <element name="lease"> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index a918679..9296ff0 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -560,6 +560,12 @@ VIR_ENUM_IMPL(virDomainNumatuneMemMode, VIR_DOMAIN_NUMATUNE_MEM_LAST, "preferred", "interleave"); +VIR_ENUM_IMPL(virDomainDeviceMigrationOptional, + VIR_DOMAIN_DEVICE_MIGRATION_OPT_LAST, + "default", + "yes", + "no"); + #define virDomainReportError(code, ...) \ virReportErrorHelper(VIR_FROM_DOMAIN, code, __FILE__, \ __FUNCTION__, __LINE__, __VA_ARGS__) @@ -764,6 +770,7 @@ void virDomainDiskDefFree(virDomainDiskDefPtr def) VIR_FREE(def->dst); VIR_FREE(def->driverName); VIR_FREE(def->driverType); + VIR_FREE(def->migration); virStorageEncryptionFree(def->encryption); virDomainDeviceInfoClear(&def->info); @@ -2247,6 +2254,56 @@ cleanup: } +static int +virDomainDeviceMigrationParseXML(xmlNodePtr node, + virDomainDiskDefPtr def) +{ + int ret = -1; + char *optional = NULL; + int i; + + if (!node || !def) { + virDomainReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("invalid argument supplied")); + return -1; + } + + if (VIR_ALLOC(def->migration) < 0) { + virReportOOMError(); + return -1; + } + + if ((optional = virXMLPropString(node, "optional")) != NULL) { + i = virDomainDeviceMigrationOptionalTypeFromString(optional); + if (i <= 0) { + virDomainReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("unknown migration optional value '%s'"), + optional); + goto cleanup; + } + + if (i == VIR_DOMAIN_DEVICE_MIGRATION_OPT_YES && + def->device != VIR_DOMAIN_DISK_DEVICE_CDROM && + def->device != VIR_DOMAIN_DISK_DEVICE_FLOPPY) { + virDomainReportError(VIR_ERR_INVALID_ARG, "%s", + _("Setting disk optional is allowed only for " + "cdrom or floppy")); + goto cleanup; + } + + def->migration->optional = i; + } + + ret = 0; + +cleanup: + if (ret < 0) + VIR_FREE(def->migration); + VIR_FREE(optional); + return ret; +} + + /* Parse the XML definition for a disk * @param node XML nodeset to parse for disk definition */ @@ -2257,7 +2314,7 @@ virDomainDiskDefParseXML(virCapsPtr caps, unsigned int flags) { virDomainDiskDefPtr def; - xmlNodePtr cur, host; + xmlNodePtr cur, host, migration = NULL; char *type = NULL; char *device = NULL; char *snapshot = NULL; @@ -2416,6 +2473,8 @@ virDomainDiskDefParseXML(virCapsPtr caps, if (virDomainDeviceBootParseXML(cur, &def->bootIndex, bootMap)) goto error; + } else if (xmlStrEqual(cur->name, BAD_CAST "migration")) { + migration = cur; } } cur = cur->next; @@ -2579,6 +2638,11 @@ virDomainDiskDefParseXML(virCapsPtr caps, def->event_idx = idx; } + if (migration) { + if (virDomainDeviceMigrationParseXML(migration, def) < 0) + goto error; + } + if (devaddr) { if (virDomainParseLegacyDeviceAddress(devaddr, &def->info.addr.pci) < 0) { @@ -9076,6 +9140,21 @@ virDomainLeaseDefFormat(virBufferPtr buf, } static int +virDomainDeviceMigrationFormat(virBufferPtr buf, + virDomainDeviceMigrationInfoPtr mig) +{ + if (!buf) + return -1; + + if (!mig) + return 0; + + virBufferAsprintf(buf, " <migration optional='%s'/>\n", + virDomainDeviceMigrationOptionalTypeToString(mig->optional)); + return 0; +} + +static int virDomainDiskDefFormat(virBufferPtr buf, virDomainDiskDefPtr def, unsigned int flags) @@ -9205,6 +9284,10 @@ virDomainDiskDefFormat(virBufferPtr buf, virStorageEncryptionFormat(buf, def->encryption, 6) < 0) return -1; + if (def->migration && + virDomainDeviceMigrationFormat(buf, def->migration) < 0) + return -1; + if (virDomainDeviceInfoFormat(buf, &def->info, flags) < 0) return -1; diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 86b4c79..4dcf511 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -259,6 +259,20 @@ enum virDomainSnapshotState { VIR_DOMAIN_DISK_SNAPSHOT = VIR_DOMAIN_LAST, }; +enum virDomainDeviceMigrationOptional { + VIR_DOMAIN_DEVICE_MIGRATION_OPT_DEFAULT = 0, + VIR_DOMAIN_DEVICE_MIGRATION_OPT_YES, + VIR_DOMAIN_DEVICE_MIGRATION_OPT_NO, + + VIR_DOMAIN_DEVICE_MIGRATION_OPT_LAST +}; + +typedef struct _virDomainDeviceMigrationInfo virDomainDeviceMigrationInfo; +typedef virDomainDeviceMigrationInfo *virDomainDeviceMigrationInfoPtr; +struct _virDomainDeviceMigrationInfo { + int optional; +}; + /* Stores the virtual disk configuration */ typedef struct _virDomainDiskDef virDomainDiskDef; typedef virDomainDiskDef *virDomainDiskDefPtr; @@ -286,6 +300,7 @@ struct _virDomainDiskDef { unsigned int transient : 1; virDomainDeviceInfo info; virStorageEncryptionPtr encryption; + virDomainDeviceMigrationInfoPtr migration; }; @@ -1893,5 +1908,6 @@ VIR_ENUM_DECL(virDomainTimerName) VIR_ENUM_DECL(virDomainTimerTrack) VIR_ENUM_DECL(virDomainTimerTickpolicy) VIR_ENUM_DECL(virDomainTimerMode) +VIR_ENUM_DECL(virDomainDeviceMigrationOptional) #endif /* __DOMAIN_CONF_H */ diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 8235ea1..bb6ee87 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -273,6 +273,8 @@ virDomainDeviceAddressTypeToString; virDomainDeviceDefFree; virDomainDeviceDefParse; virDomainDeviceInfoIterate; +virDomainDeviceMigrationOptionalTypeFromString; +virDomainDeviceMigrationOptionalTypeToString; virDomainDevicePCIAddressIsValid; virDomainDeviceTypeToString; virDomainDiskBusTypeToString; -- 1.7.3.4
On 09/27/2011 08:39 AM, Michal Privoznik wrote:
This element says what to do with cdrom (or floppy) on migration. Currently, only one attribute is supported: 'optional'. It accepts 'yes' and 'no' values. Setting a cdrom to be optional means, if destination cannot access disk source, it simply gets free()'d/ejected.
This functionality is important for users, whose machines get buggy. So they decide to save as much as possible and migrate the machine even they won't be able to access (readonly) cdrom on destination. --- docs/schemas/domaincommon.rng | 16 ++++++++
Missing docs/formatdomain.html.in, so I can't ack this. That said, I'll still review...
src/conf/domain_conf.c | 85 ++++++++++++++++++++++++++++++++++++++++- src/conf/domain_conf.h | 16 ++++++++ src/libvirt_private.syms | 2 + 4 files changed, 118 insertions(+), 1 deletions(-)
diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index be98be0..1150297 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -630,6 +630,9 @@ <ref name="encryption"/> </optional> <optional> +<ref name="migration"/> +</optional> +<optional>
This allows <migration> for all disk types. With a bit more RNG magic, it might be possible to enforce that <migration> only appears for cdrom and floppy. On the other hand, it might be possible to further extend <migration> for how it interacts with snapshots and/or copy-on-read behaviors (for example, a future patch might make it possible to request that migration creates a qcow2 wrapper on the destination with the source as its backing file, then after migration completes, perform a block pull to break the remote link), so I'm okay with leaving <migration> for all three disk types with future extensibility in mind.
+++ b/src/conf/domain_conf.c @@ -560,6 +560,12 @@ VIR_ENUM_IMPL(virDomainNumatuneMemMode, VIR_DOMAIN_NUMATUNE_MEM_LAST, "preferred", "interleave");
+VIR_ENUM_IMPL(virDomainDeviceMigrationOptional, + VIR_DOMAIN_DEVICE_MIGRATION_OPT_LAST, + "default", + "yes", + "no");
Are these the right values for the attribute? I see at least three choices, not two: 1. require - destination must see the same cd as source 2. optional - if destination has same cd, use it, if not, eject 3. drop - on migration, destination will eject, even if it has same cd Also, does this play well with cdroms that the host has locked? That is, since there is a difference between eject (which fails if guest has lock) and eject -f (which succeeds at ripping out the disk regardless of the guest's current use of the disk), we may need to factor in some description of whether we use force, or whether the migration fails if a guest lock is in place, and so forth.
+ #define virDomainReportError(code, ...) \ virReportErrorHelper(VIR_FROM_DOMAIN, code, __FILE__, \ __FUNCTION__, __LINE__, __VA_ARGS__) @@ -764,6 +770,7 @@ void virDomainDiskDefFree(virDomainDiskDefPtr def) VIR_FREE(def->dst); VIR_FREE(def->driverName); VIR_FREE(def->driverType); + VIR_FREE(def->migration);
Store def->migration by value, not by reference, and life gets simpler. More on this later...
virStorageEncryptionFree(def->encryption); virDomainDeviceInfoClear(&def->info);
@@ -2247,6 +2254,56 @@ cleanup: }
+static int +virDomainDeviceMigrationParseXML(xmlNodePtr node, + virDomainDiskDefPtr def) +{ + int ret = -1; + char *optional = NULL; + int i; + + if (!node || !def) { + virDomainReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("invalid argument supplied")); + return -1; + } + + if (VIR_ALLOC(def->migration)< 0) { + virReportOOMError(); + return -1; + }
No need to alloc if you store by value.
@@ -9076,6 +9140,21 @@ virDomainLeaseDefFormat(virBufferPtr buf, }
static int +virDomainDeviceMigrationFormat(virBufferPtr buf, + virDomainDeviceMigrationInfoPtr mig) +{ + if (!buf) + return -1; + + if (!mig) + return 0; + + virBufferAsprintf(buf, "<migration optional='%s'/>\n", + virDomainDeviceMigrationOptionalTypeToString(mig->optional));
We shall see whether this needs a tweak once my v2 indentation patches are in :)
+ +typedef struct _virDomainDeviceMigrationInfo virDomainDeviceMigrationInfo; +typedef virDomainDeviceMigrationInfo *virDomainDeviceMigrationInfoPtr; +struct _virDomainDeviceMigrationInfo { + int optional;
I guess it makes sense to break this into a separate struct, given my argument above that we may extend <migration> further for other migration-related aspects of how to handle disks.
+}; + /* Stores the virtual disk configuration */ typedef struct _virDomainDiskDef virDomainDiskDef; typedef virDomainDiskDef *virDomainDiskDefPtr; @@ -286,6 +300,7 @@ struct _virDomainDiskDef { unsigned int transient : 1; virDomainDeviceInfo info; virStorageEncryptionPtr encryption; + virDomainDeviceMigrationInfoPtr migration;
Rather than storing a virDomainDeviceMigrationInfoPtr that needs migration, you can get away with virDomainDeviceMigrationInfo by value. Allocating a pointer to a sub-struct is only needed when we want to be able to easily pass around just the sub-struct, but I don't see that happening here. I think the biggest hurdle to clear is to get consensus on the xml representation - I think your overall idea of dropping the disk source of ejectable disks on migration makes sense, but am not convinced we have the best xml representation for that notion yet. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
This patch implements previous extension in qemu driver. That is, during prepare phase check for every source to be accessible. If not, but marked as optional, simply VIR_FREE the source. Moreover, if migration is persistent, we must check inactive xml, which is transfered at the finish phase, as well. --- src/qemu/qemu_migration.c | 47 +++++++++++++++++++++++++++++++++++++++++++- 1 files changed, 45 insertions(+), 2 deletions(-) diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index ef48d65..f794ffd 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -1039,6 +1039,44 @@ cleanup: return rv; } +/* qemuCheckDisksPresence: + * @def domain definition + * + * Iterate over domain disks and check if source exists. + * If not and: + * - it's marked as optional, free() it. + * - it's not optional, throw an error. + * + * Returns 0 on success (all remaining disks/sources exist), + * -1 on failure. + */ +static int +qemuCheckDisksPresence(virDomainDefPtr def) { + int ret = -1; + int i; + virDomainDiskDefPtr disk; + + for (i = 0; i < def->ndisks; i++) { + disk = def->disks[i]; + + if (virFileExists(disk->src)) + continue; + + if (!disk->migration || + (disk->migration->optional != VIR_DOMAIN_DEVICE_MIGRATION_OPT_YES)) { + qemuReportError(VIR_ERR_NO_SOURCE, + _("no such file %s"), + disk->src); + goto cleanup; + } + + VIR_FREE(disk->src); + } + + ret = 0; +cleanup: + return ret; +} /* Prepare is the first step, and it runs on the destination host. */ @@ -1087,6 +1125,9 @@ qemuMigrationPrepareAny(struct qemud_driver *driver, if (virDomainObjIsDuplicate(&driver->domains, def, 1) < 0) goto cleanup; + if (qemuCheckDisksPresence(def) < 0) + goto cleanup; + if (!(vm = virDomainAssignDef(driver->caps, &driver->domains, def, true))) { @@ -2578,9 +2619,11 @@ qemuMigrationFinish(struct qemud_driver *driver, if (vm->persistent) newVM = 0; vm->persistent = 1; - if (mig->persistent) + if (mig->persistent) { + if (qemuCheckDisksPresence(mig->persistent) < 0) + goto endjob; vm->newDef = vmdef = mig->persistent; - else + } else vmdef = virDomainObjGetPersistentDef(driver->caps, vm); if (virDomainSaveConfig(driver->configDir, vmdef) < 0) { /* Hmpf. Migration was successful, but making it persistent -- 1.7.3.4
participants (2)
-
Eric Blake -
Michal Privoznik