9:39 a.m.
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
9:39 a.m.
New subject: [libvirt] [PATCH 1/2] migration: Introduce <migration> element for cdrom and floppy
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
10:45 a.m.
New subject: [libvirt] [PATCH 1/2] migration: Introduce <migration> element for cdrom and floppy
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(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
9:39 a.m.
New subject: [libvirt] [PATCH 2/2] qemu: Implement migration optional disk
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
4808
days inactive
4808
days old
3 comments
2 participants
participants (2)
-
Eric Blake -
Michal Privoznik