On Mon, Oct 03, 2011 at 04:07:23PM +0200, Michal Privoznik wrote:

This element says what to do with cdrom (or floppy) on migration. Currently, only one attribute is supported: 'optional'. It accepts 'require', 'optional' and 'drop' values. Setting a cdrom to be required means migration will fail if destination cannot access source under the same path. Setting to optional means, if destination cannot access disk source, it simply gets free()'d/ejected. Finally, setting to drop will simply cause cdrom to be dropped regardless of path being accessible or not.

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.

Migration will also be disallowed when the vm uses host devices or has snapshots (qemuMigrationIsAllowed)[1]. Would it make sense to introduce a VIR_MIGRATE_FORCE similar to VIR_REVERT_FORCE here? We could then introduce error codes similar to the snapshot case (VIR_ERR_MIGRATE_RISKY). The attached patch shows how this could look like, it's not meant to be applied yet. Cheers, -- Guido (sorry for hijacking this thread a bit) [1] Hopefully we can make migration with snapshots safe in the future by transfering the metadata. A current hack around is to put this onto shared storage and reread it on the destination side after migration.

--- docs/formatdomain.html.in | 23 ++++++ docs/schemas/domaincommon.rng | 17 ++++ src/conf/domain_conf.c | 78 +++++++++++++++++++- src/conf/domain_conf.h | 17 ++++ src/libvirt_private.syms | 2 + .../domain-qemu-complex-migration.xml | 68 +++++++++++++++++ 6 files changed, 204 insertions(+), 1 deletions(-) create mode 100644 tests/domainschemadata/domain-qemu-complex-migration.xml

diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index 593adcb..c72967f 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -917,6 +917,7 @@ <driver name='qemu' type='raw'/> <target def='hdc' bus='ide'/> <readonly/> + <migration optional='require'> </disk> </devices> ...</pre> @@ -1109,6 +1110,28 @@ </tr> </table> </dd> + <dt><code>migration</code></dt> + <dd>This element says what to do with disk during migration. Currently, + only <code>optional</code> attribute is supported. This attribute + specify policy for CD ROM and floppy in cases when source has or not + access to source. Accepted values: + <table class="top_table"> + <tr> + <td> require </td> + <td> destination must see the same medium as source </td> + </tr> + <tr> + <td> optional </td> + <td> if destination has same medium, use it. If not, + eject medium regardless drive being locked or not. </td> + </tr> + <tr> + <td> drop </td> + <td> on migration, destination will eject, even if it + has same medium. Again, regardless drive lock status. </td> + </tr> + </table> + </dd> <dt><code>address</code></dt> <dd>If present, the <code>address</code> element ties the disk to a given slot of a controller (the diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index cffaac2..16a76e0 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,20 @@ </choice> </attribute> </define> + <define name="migration"> + <optional> + <element name="migration"> + <attribute name="optional"> + <choice> + <value>require</value> + <value>optional</value> + <value>drop</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 6fb1888..e1f43b6 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -572,6 +572,13 @@ VIR_ENUM_IMPL(virDomainNumatuneMemMode, VIR_DOMAIN_NUMATUNE_MEM_LAST, "preferred", "interleave");

+VIR_ENUM_IMPL(virDomainDeviceMigrationOptional, + VIR_DOMAIN_DEVICE_MIGRATION_OPT_LAST, + "default", + "require", + "optional", + "drop"); + #define virDomainReportError(code, ...) \ virReportErrorHelper(VIR_FROM_DOMAIN, code, __FILE__, \ __FUNCTION__, __LINE__, __VA_ARGS__) @@ -2273,6 +2280,50 @@ 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 ((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_REQ && + def->device != VIR_DOMAIN_DISK_DEVICE_CDROM && + def->device != VIR_DOMAIN_DISK_DEVICE_FLOPPY) { + virDomainReportError(VIR_ERR_INVALID_ARG, + _("Setting disk %s is allowed only for " + "cdrom or floppy"), + optional); + goto cleanup; + } + + def->migration.optional = i; + } + + ret = 0; + +cleanup: + VIR_FREE(optional); + return ret; +} + + /* Parse the XML definition for a disk * @param node XML nodeset to parse for disk definition */ @@ -2283,7 +2334,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; @@ -2442,6 +2493,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; @@ -2605,6 +2658,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) { @@ -9116,6 +9174,21 @@ virDomainLeaseDefFormat(virBufferPtr buf, }

static int +virDomainDeviceMigrationFormat(virBufferPtr buf, + virDomainDeviceMigrationInfoPtr mig) +{ + if (!buf || !mig) + return -1; + + if (mig->optional == VIR_DOMAIN_DEVICE_MIGRATION_OPT_DEFAULT) + return 0; + + virBufferAsprintf(buf, " <migration optional='%s'/>\n", + virDomainDeviceMigrationOptionalTypeToString(mig->optional)); + return 0; +} + +static int virDomainDiskDefFormat(virBufferPtr buf, virDomainDiskDefPtr def, unsigned int flags) @@ -9245,6 +9318,9 @@ virDomainDiskDefFormat(virBufferPtr buf, virStorageEncryptionFormat(buf, def->encryption, 6) < 0) return -1;

+ if (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 bc41d34..e0db892 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -268,6 +268,21 @@ enum virDomainSnapshotState { VIR_DOMAIN_DISK_SNAPSHOT = VIR_DOMAIN_LAST, };

+enum virDomainDeviceMigrationOptional { + VIR_DOMAIN_DEVICE_MIGRATION_OPT_DEFAULT = 0, + VIR_DOMAIN_DEVICE_MIGRATION_OPT_REQ, + VIR_DOMAIN_DEVICE_MIGRATION_OPT_OPT, + VIR_DOMAIN_DEVICE_MIGRATION_OPT_DROP, + + 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; @@ -295,6 +310,7 @@ struct _virDomainDiskDef { unsigned int transient : 1; virDomainDeviceInfo info; virStorageEncryptionPtr encryption; + virDomainDeviceMigrationInfo migration; };

@@ -1912,5 +1928,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 1ac486f..881d535 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -275,6 +275,8 @@ virDomainDeviceAddressTypeToString; virDomainDeviceDefFree; virDomainDeviceDefParse; virDomainDeviceInfoIterate; +virDomainDeviceMigrationOptionalTypeFromString; +virDomainDeviceMigrationOptionalTypeToString; virDomainDevicePCIAddressIsValid; virDomainDeviceTypeToString; virDomainDiskBusTypeToString; diff --git a/tests/domainschemadata/domain-qemu-complex-migration.xml b/tests/domainschemadata/domain-qemu-complex-migration.xml new file mode 100644 index 0000000..7071b3d --- /dev/null +++ b/tests/domainschemadata/domain-qemu-complex-migration.xml @@ -0,0 +1,68 @@ +<domain type='kvm'> + <name>pxe</name> + <uuid>ee74a5f1-71c1-4a46-84a9-05ac9199fe8f</uuid> + <memory>1048576</memory> + <currentMemory>1048576</currentMemory> + <vcpu>4</vcpu> + <os> + <type arch='x86_64' machine='pc-0.14'>hvm</type> + <boot dev='network'/> + </os> + <features> + <acpi/> + <apic/> + <pae/> + </features> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>restart</on_crash> + <devices> + <emulator>/usr/bin/qemu-kvm</emulator> + <disk type='file' device='cdrom'> + <driver name='qemu' type='raw'/> + <source file='/tmp/cdrom.iso'/> + <target dev='hdc' bus='ide'/> + <readonly/> + <migration optional='optional'/> + <address type='drive' controller='0' bus='1' unit='0'/> + </disk> + <disk type='block' device='disk'> + <driver name='qemu' type='raw'/> + <source dev='/tmp/disk.img'/> + <target dev='hdd' bus='ide'/> + <migration optional='require'/> + <address type='drive' controller='0' bus='1' unit='1'/> + </disk> + <controller type='virtio-serial' index='0'> + <address type='pci' domain='0x0000' bus='0x00' slot='0x05' function='0x0'/> + </controller> + <controller type='ide' index='0'> + <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x1'/> + </controller> + <interface type='network'> + <mac address='52:54:00:75:71:fe'/> + <source network='default'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x04' function='0x0'/> + </interface> + <serial type='pty'> + <target port='0'/> + </serial> + <console type='pty'> + <target type='serial' port='0'/> + </console> + <channel type='spicevmc'> + <target type='virtio' name='com.redhat.spice.0'/> + <address type='virtio-serial' controller='0' bus='0' port='1'/> + </channel> + <input type='mouse' bus='ps2'/> + <graphics type='spice' port='5999' autoport='no' keymap='en-us'/> + <video> + <model type='cirrus' vram='9216' heads='1'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x02' function='0x0'/> + </video> + <memballoon model='virtio'> + <address type='pci' domain='0x0000' bus='0x00' slot='0x03' function='0x0'/> + </memballoon> + </devices> +</domain> -- 1.7.3.4

-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list

On Mon, Oct 03, 2011 at 04:07:24PM +0200, Michal Privoznik wrote:

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 | 51 +++++++++++++++++++++++++++++++++++++++++++- 1 files changed, 49 insertions(+), 2 deletions(-)

diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index 1122dab..4aa2532 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -1039,6 +1039,48 @@ 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 marked as required, throw an error. + * + * Returns 0 on success (all remaining disks/sources exist + * or have been dropped), + * -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)) { + if (disk->migration.optional == VIR_DOMAIN_DEVICE_MIGRATION_OPT_DROP) + VIR_FREE(disk->src); + + continue; + }

This is not going to play nice with RHEVM, where images are stored on a root squashing NFS server, where libvirtd has no visibility, but QEMU can access. At the very least you need to run this check in a separate process which is running as the QEMU user/group ID.

+ + if (disk->migration.optional == VIR_DOMAIN_DEVICE_MIGRATION_OPT_REQ) { + 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 +1129,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))) { @@ -2579,9 +2624,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;

What's the point in checking here ? You've already checked at te start of migration, and if it has gone away since then, it is too late for this check todo anything useful 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 :|