[libvirt] [PATCH v2 00/11] Add support for VM Generation ID (vmgenid)
v1: https://www.redhat.com/archives/libvir-list/2018-April/msg00913.html Changes since v1: - Remove the qemuProcessCheckGenID and clean up from the fallout of that - Alter the *.args based on all the recent caps default change adjustments John Ferlan (11): Check return status for virUUIDGenerate conf: Modify last arg to virDomainDefCopy qemu: Add flags to qemuDomainMigratableDefCheckABIStability conf: Add VM Generation ID to virDomainDef conf: Add flag to regenerate genid for virDomainDefCopy conf: Add VM Generation ID parse/format support qemu: Add VM Generation ID device capability qemu: Handle genid processing during startup/launch Remove check for duplicitous GenID in another VM qemu: Add VM Generation ID to qemu command line docs: Add news article for VM Generation ID docs/formatdomain.html.in | 29 ++++++++ docs/news.xml | 12 ++++ docs/schemas/domaincommon.rng | 8 +++ src/conf/domain_conf.c | 81 +++++++++++++++++++++-- src/conf/domain_conf.h | 19 +++++- src/conf/network_conf.c | 2 +- src/conf/secret_conf.c | 2 +- src/libxl/libxl_domain.c | 4 +- src/openvz/openvz_conf.c | 2 +- src/qemu/qemu_capabilities.c | 2 + src/qemu/qemu_capabilities.h | 1 + src/qemu/qemu_command.c | 31 +++++++++ src/qemu/qemu_domain.c | 23 ++++--- src/qemu/qemu_domain.h | 6 +- src/qemu/qemu_driver.c | 31 ++++++--- src/qemu/qemu_migration.c | 4 +- src/qemu/qemu_process.c | 46 ++++++++++++- src/qemu/qemu_process.h | 1 + src/test/test_driver.c | 6 +- src/xenconfig/xen_common.c | 2 +- tests/qemucapabilitiesdata/caps_2.10.0.x86_64.xml | 1 + tests/qemucapabilitiesdata/caps_2.12.0.x86_64.xml | 1 + tests/qemucapabilitiesdata/caps_2.9.0.x86_64.xml | 1 + tests/qemuxml2argvdata/genid-auto.args | 24 +++++++ tests/qemuxml2argvdata/genid-auto.xml | 32 +++++++++ tests/qemuxml2argvdata/genid.args | 24 +++++++ tests/qemuxml2argvdata/genid.xml | 32 +++++++++ tests/qemuxml2argvtest.c | 4 ++ tests/qemuxml2xmloutdata/genid-active.xml | 32 +++++++++ tests/qemuxml2xmloutdata/genid-auto-active.xml | 32 +++++++++ tests/qemuxml2xmloutdata/genid-auto-inactive.xml | 32 +++++++++ tests/qemuxml2xmloutdata/genid-inactive.xml | 32 +++++++++ tests/qemuxml2xmltest.c | 5 +- 33 files changed, 527 insertions(+), 37 deletions(-) create mode 100644 tests/qemuxml2argvdata/genid-auto.args create mode 100644 tests/qemuxml2argvdata/genid-auto.xml create mode 100644 tests/qemuxml2argvdata/genid.args create mode 100644 tests/qemuxml2argvdata/genid.xml create mode 100644 tests/qemuxml2xmloutdata/genid-active.xml create mode 100644 tests/qemuxml2xmloutdata/genid-auto-active.xml create mode 100644 tests/qemuxml2xmloutdata/genid-auto-inactive.xml create mode 100644 tests/qemuxml2xmloutdata/genid-inactive.xml -- 2.13.6
Although legal, a few paths were not checking a return value < 0 for failure instead they checked a non zero failure. Clean them all up to be consistent. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/domain_conf.c | 2 +- src/conf/network_conf.c | 2 +- src/conf/secret_conf.c | 2 +- src/openvz/openvz_conf.c | 2 +- src/xenconfig/xen_common.c | 2 +- 5 files changed, 5 insertions(+), 5 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 65b429460a..dacf888565 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -18778,7 +18778,7 @@ virDomainDefParseXML(xmlDocPtr xml, * also serve as the uuid. */ tmp = virXPathString("string(./uuid[1])", ctxt); if (!tmp) { - if (virUUIDGenerate(def->uuid)) { + if (virUUIDGenerate(def->uuid) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Failed to generate UUID")); goto error; diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c index 54109a3d2e..630a87fc07 100644 --- a/src/conf/network_conf.c +++ b/src/conf/network_conf.c @@ -1619,7 +1619,7 @@ virNetworkDefParseXML(xmlXPathContextPtr ctxt) /* Extract network uuid */ tmp = virXPathString("string(./uuid[1])", ctxt); if (!tmp) { - if (virUUIDGenerate(def->uuid)) { + if (virUUIDGenerate(def->uuid) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Failed to generate UUID")); goto error; diff --git a/src/conf/secret_conf.c b/src/conf/secret_conf.c index 989705234c..7a2e4b28aa 100644 --- a/src/conf/secret_conf.c +++ b/src/conf/secret_conf.c @@ -177,7 +177,7 @@ secretXMLParseNode(xmlDocPtr xml, xmlNodePtr root) uuidstr = virXPathString("string(./uuid)", ctxt); if (!uuidstr) { - if (virUUIDGenerate(def->uuid)) { + if (virUUIDGenerate(def->uuid) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Failed to generate UUID")); goto cleanup; diff --git a/src/openvz/openvz_conf.c b/src/openvz/openvz_conf.c index a25eaf570e..72ef63a355 100644 --- a/src/openvz/openvz_conf.c +++ b/src/openvz/openvz_conf.c @@ -1019,7 +1019,7 @@ openvzSetUUID(int vpsid) { unsigned char uuid[VIR_UUID_BUFLEN]; - if (virUUIDGenerate(uuid)) { + if (virUUIDGenerate(uuid) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Failed to generate UUID")); return -1; diff --git a/src/xenconfig/xen_common.c b/src/xenconfig/xen_common.c index bc43185363..7db365f363 100644 --- a/src/xenconfig/xen_common.c +++ b/src/xenconfig/xen_common.c @@ -202,7 +202,7 @@ xenConfigGetUUID(virConfPtr conf, const char *name, unsigned char *uuid) } if (!(val = virConfGetValue(conf, name))) { - if (virUUIDGenerate(uuid)) { + if (virUUIDGenerate(uuid) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Failed to generate UUID")); return -1; -- 2.13.6
On Mon, Apr 23, 2018 at 19:59:55 -0400, John Ferlan wrote:
Although legal, a few paths were not checking a return value < 0 for failure instead they checked a non zero failure.
Clean them all up to be consistent.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/domain_conf.c | 2 +- src/conf/network_conf.c | 2 +- src/conf/secret_conf.c | 2 +- src/openvz/openvz_conf.c | 2 +- src/xenconfig/xen_common.c | 2 +- 5 files changed, 5 insertions(+), 5 deletions(-)
ACK
Rather than single boolean, let's create a virDomainDefCopyFlags and have the last argument be an unsigned int using the new VIR_DOMAIN_DEF_COPY_MIGRATABLE bit. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/domain_conf.c | 8 ++++---- src/conf/domain_conf.h | 6 +++++- src/libxl/libxl_domain.c | 4 ++-- src/qemu/qemu_domain.c | 5 +++-- src/qemu/qemu_driver.c | 5 +++-- src/test/test_driver.c | 5 +++-- 6 files changed, 20 insertions(+), 13 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index dacf888565..1411034e7c 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -3315,7 +3315,7 @@ virDomainObjSetDefTransient(virCapsPtr caps, if (domain->newDef) return 0; - if (!(domain->newDef = virDomainDefCopy(domain->def, caps, xmlopt, NULL, false))) + if (!(domain->newDef = virDomainDefCopy(domain->def, caps, xmlopt, NULL, 0))) goto out; ret = 0; @@ -27892,7 +27892,7 @@ virDomainDefCopy(virDomainDefPtr src, virCapsPtr caps, virDomainXMLOptionPtr xmlopt, void *parseOpaque, - bool migratable) + unsigned int flags) { char *xml; virDomainDefPtr ret; @@ -27900,7 +27900,7 @@ virDomainDefCopy(virDomainDefPtr src, unsigned int parse_flags = VIR_DOMAIN_DEF_PARSE_INACTIVE | VIR_DOMAIN_DEF_PARSE_SKIP_VALIDATE; - if (migratable) + if (flags & VIR_DOMAIN_DEF_COPY_MIGRATABLE) format_flags |= VIR_DOMAIN_DEF_FORMAT_INACTIVE | VIR_DOMAIN_DEF_FORMAT_MIGRATABLE; /* Easiest to clone via a round-trip through XML. */ @@ -27921,7 +27921,7 @@ virDomainObjCopyPersistentDef(virDomainObjPtr dom, virDomainDefPtr cur; cur = virDomainObjGetPersistentDef(caps, xmlopt, dom); - return virDomainDefCopy(cur, caps, xmlopt, NULL, false); + return virDomainDefCopy(cur, caps, xmlopt, NULL, 0); } diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 3c7eccb8ca..99d1aa529a 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -2867,11 +2867,15 @@ virDomainDefPtr virDomainObjGetOneDefState(virDomainObjPtr vm, bool *state); virDomainDefPtr virDomainObjGetOneDef(virDomainObjPtr vm, unsigned int flags); +typedef enum { + /* Set when creating a copy of a definition for the purpose of migration */ + VIR_DOMAIN_DEF_COPY_MIGRATABLE = 1 << 0, +} virDomainDefCopyFlags; virDomainDefPtr virDomainDefCopy(virDomainDefPtr src, virCapsPtr caps, virDomainXMLOptionPtr xmlopt, void *parseOpaque, - bool migratable); + unsigned int flags); virDomainDefPtr virDomainObjCopyPersistentDef(virDomainObjPtr dom, virCapsPtr caps, virDomainXMLOptionPtr xmlopt); diff --git a/src/libxl/libxl_domain.c b/src/libxl/libxl_domain.c index d4859d6707..126d08b499 100644 --- a/src/libxl/libxl_domain.c +++ b/src/libxl/libxl_domain.c @@ -1450,8 +1450,8 @@ libxlDomainDefCheckABIStability(libxlDriverPrivatePtr driver, libxlDriverConfigPtr cfg = libxlDriverConfigGet(driver); bool ret = false; - if (!(migratableDefSrc = virDomainDefCopy(src, cfg->caps, driver->xmlopt, NULL, true)) || - !(migratableDefDst = virDomainDefCopy(dst, cfg->caps, driver->xmlopt, NULL, true))) + if (!(migratableDefSrc = virDomainDefCopy(src, cfg->caps, driver->xmlopt, NULL, VIR_DOMAIN_DEF_COPY_MIGRATABLE)) || + !(migratableDefDst = virDomainDefCopy(dst, cfg->caps, driver->xmlopt, NULL, VIR_DOMAIN_DEF_COPY_MIGRATABLE))) goto cleanup; ret = virDomainDefCheckABIStability(migratableDefSrc, diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 326c939c85..ed7bd0230e 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -6198,6 +6198,8 @@ qemuDomainDefFormatBufInternal(virQEMUDriverPtr driver, virDomainDefPtr copy = NULL; virCapsPtr caps = NULL; virQEMUCapsPtr qemuCaps = NULL; + unsigned int copyFlags = (flags & VIR_DOMAIN_XML_MIGRATABLE) ? + VIR_DOMAIN_DEF_COPY_MIGRATABLE : 0; if (!(caps = virQEMUDriverGetCapabilities(driver, false))) goto cleanup; @@ -6205,8 +6207,7 @@ qemuDomainDefFormatBufInternal(virQEMUDriverPtr driver, if (!(flags & (VIR_DOMAIN_XML_UPDATE_CPU | VIR_DOMAIN_XML_MIGRATABLE))) goto format; - if (!(copy = virDomainDefCopy(def, caps, driver->xmlopt, NULL, - flags & VIR_DOMAIN_XML_MIGRATABLE))) + if (!(copy = virDomainDefCopy(def, caps, driver->xmlopt, NULL, copyFlags))) goto cleanup; def = copy; diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 7484b00e23..2863508189 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -15857,7 +15857,8 @@ qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot, snap->def->current = true; if (snap->def->dom) { config = virDomainDefCopy(snap->def->dom, caps, - driver->xmlopt, NULL, true); + driver->xmlopt, NULL, + VIR_DOMAIN_DEF_COPY_MIGRATABLE); if (!config) goto endjob; } @@ -20443,7 +20444,7 @@ qemuDomainGetFSInfo(virDomainPtr dom, if (!(caps = virQEMUDriverGetCapabilities(driver, false))) goto endjob; - if (!(def = virDomainDefCopy(vm->def, caps, driver->xmlopt, NULL, false))) + if (!(def = virDomainDefCopy(vm->def, caps, driver->xmlopt, NULL, 0))) goto endjob; agent = qemuDomainObjEnterAgent(vm); diff --git a/src/test/test_driver.c b/src/test/test_driver.c index a1888c0c9f..8877a467f6 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -6434,7 +6434,7 @@ testDomainSnapshotCreateXML(virDomainPtr domain, privconn->caps, privconn->xmlopt, NULL, - true))) + VIR_DOMAIN_DEF_COPY_MIGRATABLE))) goto cleanup; if (testDomainSnapshotAlignDisks(vm, def, flags) < 0) @@ -6689,7 +6689,8 @@ testDomainRevertToSnapshot(virDomainSnapshotPtr snapshot, snap->def->current = true; config = virDomainDefCopy(snap->def->dom, privconn->caps, - privconn->xmlopt, NULL, true); + privconn->xmlopt, NULL, + VIR_DOMAIN_DEF_COPY_MIGRATABLE); if (!config) goto cleanup; -- 2.13.6
Allow the caller to qemuDomainMigratableDefCheckABIStability to also provide a flag to be used for virDomainDefCheckABIStabilityFlags. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_domain.c | 18 ++++++++++++------ src/qemu/qemu_domain.h | 6 ++++-- src/qemu/qemu_driver.c | 7 ++++--- src/qemu/qemu_migration.c | 4 ++-- 4 files changed, 22 insertions(+), 13 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index ed7bd0230e..f584768895 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -8013,12 +8013,14 @@ qemuDomainMigratableDefCheckABIStability(virQEMUDriverPtr driver, virDomainDefPtr src, virDomainDefPtr migratableSrc, virDomainDefPtr dst, - virDomainDefPtr migratableDst) + virDomainDefPtr migratableDst, + unsigned flags) { + flags |= VIR_DOMAIN_DEF_ABI_CHECK_SKIP_VOLATILE; if (!virDomainDefCheckABIStabilityFlags(migratableSrc, migratableDst, driver->xmlopt, - VIR_DOMAIN_DEF_ABI_CHECK_SKIP_VOLATILE)) + flags)) return false; /* Force update any skipped values from the volatile flag */ @@ -8034,7 +8036,8 @@ qemuDomainMigratableDefCheckABIStability(virQEMUDriverPtr driver, bool qemuDomainDefCheckABIStability(virQEMUDriverPtr driver, virDomainDefPtr src, - virDomainDefPtr dst) + virDomainDefPtr dst, + unsigned int flags) { virDomainDefPtr migratableDefSrc = NULL; virDomainDefPtr migratableDefDst = NULL; @@ -8046,7 +8049,8 @@ qemuDomainDefCheckABIStability(virQEMUDriverPtr driver, ret = qemuDomainMigratableDefCheckABIStability(driver, src, migratableDefSrc, - dst, migratableDefDst); + dst, migratableDefDst, + flags); cleanup: virDomainDefFree(migratableDefSrc); @@ -8058,7 +8062,8 @@ qemuDomainDefCheckABIStability(virQEMUDriverPtr driver, bool qemuDomainCheckABIStability(virQEMUDriverPtr driver, virDomainObjPtr vm, - virDomainDefPtr dst) + virDomainDefPtr dst, + unsigned int flags) { virDomainDefPtr migratableSrc = NULL; virDomainDefPtr migratableDst = NULL; @@ -8072,7 +8077,8 @@ qemuDomainCheckABIStability(virQEMUDriverPtr driver, ret = qemuDomainMigratableDefCheckABIStability(driver, vm->def, migratableSrc, - dst, migratableDst); + dst, migratableDst, + flags); cleanup: VIR_FREE(xml); diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h index 2c27dfb9f3..789e7b403c 100644 --- a/src/qemu/qemu_domain.h +++ b/src/qemu/qemu_domain.h @@ -719,11 +719,13 @@ int qemuDomainUpdateMemoryDeviceInfo(virQEMUDriverPtr driver, bool qemuDomainDefCheckABIStability(virQEMUDriverPtr driver, virDomainDefPtr src, - virDomainDefPtr dst); + virDomainDefPtr dst, + unsigned int flags); bool qemuDomainCheckABIStability(virQEMUDriverPtr driver, virDomainObjPtr vm, - virDomainDefPtr dst); + virDomainDefPtr dst, + unsigned int flags); bool qemuDomainAgentAvailable(virDomainObjPtr vm, bool reportError); diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 2863508189..61e49bea24 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -3366,7 +3366,7 @@ qemuDomainSaveInternal(virQEMUDriverPtr driver, VIR_DOMAIN_DEF_PARSE_SKIP_VALIDATE))) { goto endjob; } - if (!qemuDomainCheckABIStability(driver, vm, def)) { + if (!qemuDomainCheckABIStability(driver, vm, def, 0)) { virDomainDefFree(def); goto endjob; } @@ -15893,9 +15893,10 @@ qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot, goto endjob; compatible = qemuDomainDefCheckABIStability(driver, vm->def, - config); + config, 0); } else { - compatible = qemuDomainCheckABIStability(driver, vm, config); + compatible = qemuDomainCheckABIStability(driver, vm, config, + 0); } if (!compatible) { diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index 88b8253fa9..e1aa3a3368 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -1954,7 +1954,7 @@ qemuMigrationSrcBeginPhase(virQEMUDriverPtr driver, VIR_DOMAIN_DEF_PARSE_SKIP_VALIDATE))) goto cleanup; - if (!qemuDomainCheckABIStability(driver, vm, def)) + if (!qemuDomainCheckABIStability(driver, vm, def, 0)) goto cleanup; rv = qemuDomainDefFormatLive(driver, def, NULL, false, true); @@ -2258,7 +2258,7 @@ qemuMigrationDstPrepareAny(virQEMUDriverPtr driver, if (!newdef) goto cleanup; - if (!qemuDomainDefCheckABIStability(driver, *def, newdef)) { + if (!qemuDomainDefCheckABIStability(driver, *def, newdef, 0)) { virDomainDefFree(newdef); goto cleanup; } -- 2.13.6
Add a genid[] field to be able to store a uuid-like GUID value. Also add associated bool's to support VM Generation ID operation. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/domain_conf.h | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 99d1aa529a..1b61fd8c03 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -2325,6 +2325,11 @@ struct _virDomainDef { virDomainVirtType virtType; int id; unsigned char uuid[VIR_UUID_BUFLEN]; + + unsigned char genid[VIR_UUID_BUFLEN]; + bool genidRequested; + bool genidGenerated; + char *name; char *title; char *description; -- 2.13.6
Add the VIR_DOMAIN_DEF_COPY_NEWGENID to indicate that the generated domain definition should adjust the genid value before returning the @def to the caller. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/domain_conf.c | 12 ++++++++++++ src/conf/domain_conf.h | 5 +++++ src/qemu/qemu_driver.c | 3 ++- src/test/test_driver.c | 3 ++- 4 files changed, 21 insertions(+), 2 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 1411034e7c..6d4db50998 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -27909,6 +27909,18 @@ virDomainDefCopy(virDomainDefPtr src, ret = virDomainDefParseString(xml, caps, xmlopt, parseOpaque, parse_flags); + /* If we have a genid and we're being called from a path that would + * require a different genid value, then regardless of whether it was + * generated or not generate a new one. */ + if (ret && ret->genidRequested && (flags & VIR_DOMAIN_DEF_COPY_NEWGENID)) { + if (virUUIDGenerate(ret->genid)) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Failed to generate a new genid")); + virDomainDefFree(ret); + ret = NULL; + } + } + VIR_FREE(xml); return ret; } diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 1b61fd8c03..910b3ca4d1 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -2875,6 +2875,11 @@ virDomainDefPtr virDomainObjGetOneDef(virDomainObjPtr vm, unsigned int flags); typedef enum { /* Set when creating a copy of a definition for the purpose of migration */ VIR_DOMAIN_DEF_COPY_MIGRATABLE = 1 << 0, + + /* Set when the copy should create a new genid value if supported + * the domain def. + */ + VIR_DOMAIN_DEF_COPY_NEWGENID = 1 << 1, } virDomainDefCopyFlags; virDomainDefPtr virDomainDefCopy(virDomainDefPtr src, virCapsPtr caps, diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 61e49bea24..bfb7973100 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -15858,7 +15858,8 @@ qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot, if (snap->def->dom) { config = virDomainDefCopy(snap->def->dom, caps, driver->xmlopt, NULL, - VIR_DOMAIN_DEF_COPY_MIGRATABLE); + VIR_DOMAIN_DEF_COPY_MIGRATABLE | + VIR_DOMAIN_DEF_COPY_NEWGENID); if (!config) goto endjob; } diff --git a/src/test/test_driver.c b/src/test/test_driver.c index 8877a467f6..7bac017e1a 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -6690,7 +6690,8 @@ testDomainRevertToSnapshot(virDomainSnapshotPtr snapshot, snap->def->current = true; config = virDomainDefCopy(snap->def->dom, privconn->caps, privconn->xmlopt, NULL, - VIR_DOMAIN_DEF_COPY_MIGRATABLE); + VIR_DOMAIN_DEF_COPY_MIGRATABLE | + VIR_DOMAIN_DEF_COPY_NEWGENID); if (!config) goto cleanup; -- 2.13.6
On Mon, Apr 23, 2018 at 19:59:59 -0400, John Ferlan wrote:
Add the VIR_DOMAIN_DEF_COPY_NEWGENID to indicate that the generated domain definition should adjust the genid value before returning the @def to the caller.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/domain_conf.c | 12 ++++++++++++ src/conf/domain_conf.h | 5 +++++ src/qemu/qemu_driver.c | 3 ++- src/test/test_driver.c | 3 ++- 4 files changed, 21 insertions(+), 2 deletions(-)
IMO all this code should not be necessary. Code paths which knowingly need to change the GENID should do so explicitly rather than pulling the logic here.
On 04/24/2018 03:24 AM, Peter Krempa wrote:
On Mon, Apr 23, 2018 at 19:59:59 -0400, John Ferlan wrote:
Add the VIR_DOMAIN_DEF_COPY_NEWGENID to indicate that the generated domain definition should adjust the genid value before returning the @def to the caller.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/domain_conf.c | 12 ++++++++++++ src/conf/domain_conf.h | 5 +++++ src/qemu/qemu_driver.c | 3 ++- src/test/test_driver.c | 3 ++- 4 files changed, 21 insertions(+), 2 deletions(-)
IMO all this code should not be necessary. Code paths which knowingly need to change the GENID should do so explicitly rather than pulling the logic here.
Honestly, I found it easier to have the code in one place rather than cut-copy-paste in two places... But I can move it out. However, I may have forgotten a transition though w/r/t domain copy. Unfortunately clone could be a bugger since it uses parse and format, but if startup always generates one, then that's perhaps covered. John
On Tue, Apr 24, 2018 at 14:13:15 -0400, John Ferlan wrote:
On 04/24/2018 03:24 AM, Peter Krempa wrote:
On Mon, Apr 23, 2018 at 19:59:59 -0400, John Ferlan wrote:
Add the VIR_DOMAIN_DEF_COPY_NEWGENID to indicate that the generated domain definition should adjust the genid value before returning the @def to the caller.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/domain_conf.c | 12 ++++++++++++ src/conf/domain_conf.h | 5 +++++ src/qemu/qemu_driver.c | 3 ++- src/test/test_driver.c | 3 ++- 4 files changed, 21 insertions(+), 2 deletions(-)
IMO all this code should not be necessary. Code paths which knowingly need to change the GENID should do so explicitly rather than pulling the logic here.
Honestly, I found it easier to have the code in one place rather than cut-copy-paste in two places... But I can move it out.
The problem is that the copying of domain definition should not attempt any smarts. The caller expects a copy, so it should be the same. If the caller wishes to use the definition in a different way it needs to be modified afterwards.
However, I may have forgotten a transition though w/r/t domain copy. Unfortunately clone could be a bugger since it uses parse and format, but if startup always generates one, then that's perhaps covered.
Copying of the domain definition should always copy the GUID along with the flag whether it was autogenerated or not. You may need a XML attribute for that. Otherwise you will not be able to differentiate the situation when the GUID was provided by the user, or when it was autogenerated but needs to be used as-is.
On 04/25/2018 03:42 AM, Peter Krempa wrote:
On Tue, Apr 24, 2018 at 14:13:15 -0400, John Ferlan wrote:
On 04/24/2018 03:24 AM, Peter Krempa wrote:
On Mon, Apr 23, 2018 at 19:59:59 -0400, John Ferlan wrote:
Add the VIR_DOMAIN_DEF_COPY_NEWGENID to indicate that the generated domain definition should adjust the genid value before returning the @def to the caller.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/domain_conf.c | 12 ++++++++++++ src/conf/domain_conf.h | 5 +++++ src/qemu/qemu_driver.c | 3 ++- src/test/test_driver.c | 3 ++- 4 files changed, 21 insertions(+), 2 deletions(-)
IMO all this code should not be necessary. Code paths which knowingly need to change the GENID should do so explicitly rather than pulling the logic here.
Honestly, I found it easier to have the code in one place rather than cut-copy-paste in two places... But I can move it out.
The problem is that the copying of domain definition should not attempt any smarts. The caller expects a copy, so it should be the same. If the caller wishes to use the definition in a different way it needs to be modified afterwards.
That ship already sailed by passing migratable, hence why it seemed fine to use the function. But like I said, I'll move it out. Although from this discussion it appears parseOpaque could be unnecessary - all callers pass NULL in the 4th parameter. Still it could be well enough hidden in some other way... John
However, I may have forgotten a transition though w/r/t domain copy. Unfortunately clone could be a bugger since it uses parse and format, but if startup always generates one, then that's perhaps covered.
Copying of the domain definition should always copy the GUID along with the flag whether it was autogenerated or not. You may need a XML attribute for that. Otherwise you will not be able to differentiate the situation when the GUID was provided by the user, or when it was autogenerated but needs to be used as-is.
The VM Generation ID is a mechanism to provide a unique 128-bit, cryptographically random, and integer value identifier known as the GUID (Globally Unique Identifier) to the guest OS. The value is used to help notify the guest operating system when the virtual machine is executed with a different configuration. This patch adds support for a new "genid" XML element similar to the "uuid" element. The "genid" element can have two forms "<genid/>" or "<genid>$GUID</genid>". If the $GUID is not provided, libvirt will generate one. For the ABI checks add avoidance for the genid comparison if the appropriate flag is set. Since adding support for a generated GUID (or UUID like) value to be displayed only for an active guest, modifying the xml2xml test to include virrandommock.so is necessary since it will generate a "known" UUID value that can be compared against for the active test. Signed-off-by: John Ferlan <jferlan@redhat.com> --- docs/formatdomain.html.in | 29 ++++++++++++ docs/schemas/domaincommon.rng | 8 ++++ src/conf/domain_conf.c | 59 ++++++++++++++++++++++++ src/conf/domain_conf.h | 3 ++ tests/qemuxml2argvdata/genid-auto.xml | 32 +++++++++++++ tests/qemuxml2argvdata/genid.xml | 32 +++++++++++++ tests/qemuxml2xmloutdata/genid-active.xml | 32 +++++++++++++ tests/qemuxml2xmloutdata/genid-auto-active.xml | 32 +++++++++++++ tests/qemuxml2xmloutdata/genid-auto-inactive.xml | 32 +++++++++++++ tests/qemuxml2xmloutdata/genid-inactive.xml | 32 +++++++++++++ tests/qemuxml2xmltest.c | 5 +- 11 files changed, 295 insertions(+), 1 deletion(-) create mode 100644 tests/qemuxml2argvdata/genid-auto.xml create mode 100644 tests/qemuxml2argvdata/genid.xml create mode 100644 tests/qemuxml2xmloutdata/genid-active.xml create mode 100644 tests/qemuxml2xmloutdata/genid-auto-active.xml create mode 100644 tests/qemuxml2xmloutdata/genid-auto-inactive.xml create mode 100644 tests/qemuxml2xmloutdata/genid-inactive.xml diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index ada0df227f..6a140f3e40 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -34,6 +34,7 @@ <domain type='kvm' id='1'> <name>MyGuest</name> <uuid>4dea22b3-1d52-d8f3-2516-782e98ab3fa0</uuid> + <genid>43dc0cf8-809b-4adb-9bea-a9abb5f3d90e</genid> <title>A short description - title - of the domain</title> <description>Some human readable description</description> <metadata> @@ -61,6 +62,34 @@ specification. <span class="since">Since 0.0.1, sysinfo since 0.8.7</span></dd> + <dt><code>genid</code></dt> + <dd><span class="since">Since 4.3.0</span>, the <code>genid</code> + element can be used to add a Virtual Machine Generation ID which + exposes a 128-bit, cryptographically random, integer value identifier, + referred to as a Globally Unique Identifier (GUID) using the same + format as the <code>uuid</code>. The value is used to help notify + the guest operating system when the virtual machine is executed + with a different configuration, such as: + + <ul> + <li>snapshot execution</li> + <li>backup recovery</li> + <li>failover in a disaster recovery environment</li> + <li>creation from template (import, copy, clone)</li> + </ul> + + The guest operating system notices the change and is then able to + react as appropriate by marking its copies of distributed databases + as dirty, re-initializing its random number generator, etc. + + <p> + When a GUID value is not provided, e.g. using the XML syntax + <genid/>, then libvirt will automatically generate a GUID. + This is the recommended configuration since the hypervisor then + can handle changing the GUID value for specific state transitions. + Using a static GUID value may result in failures for starting from + snapshot, restoring from backup, starting a cloned domain, etc.</p></dd> + <dt><code>title</code></dt> <dd>The optional element <code>title</code> provides space for a short description of the domain. The title should not contain diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index 4cab55f05d..1892a7c63c 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -502,6 +502,14 @@ <ref name="UUID"/> </element> </optional> + <optional> + <element name="genid"> + <choice> + <ref name="UUID"/> + <empty/> + </choice> + </element> + </optional> </interleave> </define> <define name="idmap"> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 6d4db50998..8c30adf850 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -18793,6 +18793,34 @@ virDomainDefParseXML(xmlDocPtr xml, VIR_FREE(tmp); } + /* Extract domain genid - a genid can either be provided or generated */ + if ((n = virXPathNodeSet("./genid", ctxt, &nodes)) < 0) + goto error; + + if (n > 0) { + if (n != 1) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("element 'genid' can only appear once")); + goto error; + } + def->genidRequested = true; + if (!(tmp = virXPathString("string(./genid[1])", ctxt))) { + if (virUUIDGenerate(def->genid) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + "%s", _("Failed to generate genid")); + goto error; + } + def->genidGenerated = true; + } else { + if (virUUIDParse(tmp, def->genid) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + "%s", _("malformed genid element")); + goto error; + } + } + } + VIR_FREE(nodes); + /* Extract short description of domain (title) */ def->title = virXPathString("string(./title[1])", ctxt); if (def->title && strchr(def->title, '\n')) { @@ -21904,6 +21932,26 @@ virDomainDefCheckABIStabilityFlags(virDomainDefPtr src, goto error; } + if (src->genidRequested != dst->genidRequested) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("Target domain requested genid does not match source")); + goto error; + } + + if (src->genidRequested && + !(flags & VIR_DOMAIN_DEF_ABI_CHECK_SKIP_GENID) && + memcmp(src->genid, dst->genid, VIR_UUID_BUFLEN) != 0) { + char guidsrc[VIR_UUID_STRING_BUFLEN]; + char guiddst[VIR_UUID_STRING_BUFLEN]; + + virUUIDFormat(src->genid, guidsrc); + virUUIDFormat(dst->genid, guiddst); + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("Target domain genid %s does not match source %s"), + guiddst, guidsrc); + goto error; + } + /* Not strictly ABI related, but we want to make sure domains * don't get silently re-named through the backdoor when passing * custom XML into various APIs, since this would create havoc @@ -26541,6 +26589,17 @@ virDomainDefFormatInternal(virDomainDefPtr def, virUUIDFormat(uuid, uuidstr); virBufferAsprintf(buf, "<uuid>%s</uuid>\n", uuidstr); + if (def->genidRequested) { + char genidstr[VIR_UUID_STRING_BUFLEN]; + + virUUIDFormat(def->genid, genidstr); + if (!def->genidGenerated || + !(flags & VIR_DOMAIN_DEF_FORMAT_INACTIVE)) + virBufferAsprintf(buf, "<genid>%s</genid>\n", genidstr); + else + virBufferAddLit(buf, "<genid/>\n"); + } + virBufferEscapeString(buf, "<title>%s</title>\n", def->title); virBufferEscapeString(buf, "<description>%s</description>\n", diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 910b3ca4d1..e9248a34c2 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -2952,6 +2952,9 @@ typedef enum { /* Set when domain lock must be released and there exists the possibility * that some external action could alter the value, such as cur_balloon. */ VIR_DOMAIN_DEF_ABI_CHECK_SKIP_VOLATILE = 1 << 0, + + /* Set when the ABI check should skip the genid comparison */ + VIR_DOMAIN_DEF_ABI_CHECK_SKIP_GENID = 1 << 1, } virDomainDefABICheckFlags; virDomainDeviceDefPtr virDomainDeviceDefParse(const char *xmlStr, diff --git a/tests/qemuxml2argvdata/genid-auto.xml b/tests/qemuxml2argvdata/genid-auto.xml new file mode 100644 index 0000000000..96ad9ddda8 --- /dev/null +++ b/tests/qemuxml2argvdata/genid-auto.xml @@ -0,0 +1,32 @@ +<domain type='qemu' id='1'> + <name>QEMUGuest1</name> + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> + <genid/> + <memory unit='KiB'>219136</memory> + <currentMemory unit='KiB'>219136</currentMemory> + <vcpu placement='static'>1</vcpu> + <os> + <type arch='x86_64' machine='pc'>hvm</type> + <boot dev='hd'/> + </os> + <features> + <acpi/> + </features> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash> + <devices> + <emulator>/usr/bin/qemu-system-x86_64</emulator> + <controller type='usb' index='0'> + <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x2'/> + </controller> + <controller type='ide' index='0'> + <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x1'/> + </controller> + <controller type='pci' index='0' model='pci-root'/> + <input type='mouse' bus='ps2'/> + <input type='keyboard' bus='ps2'/> + <memballoon model='none'/> + </devices> +</domain> diff --git a/tests/qemuxml2argvdata/genid.xml b/tests/qemuxml2argvdata/genid.xml new file mode 100644 index 0000000000..fc41f2dd28 --- /dev/null +++ b/tests/qemuxml2argvdata/genid.xml @@ -0,0 +1,32 @@ +<domain type='qemu' id='1'> + <name>QEMUGuest1</name> + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> + <genid>e9392370-2917-565e-692b-d057f46512d6</genid> + <memory unit='KiB'>219136</memory> + <currentMemory unit='KiB'>219136</currentMemory> + <vcpu placement='static'>1</vcpu> + <os> + <type arch='x86_64' machine='pc'>hvm</type> + <boot dev='hd'/> + </os> + <features> + <acpi/> + </features> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash> + <devices> + <emulator>/usr/bin/qemu-system-x86_64</emulator> + <controller type='usb' index='0'> + <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x2'/> + </controller> + <controller type='ide' index='0'> + <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x1'/> + </controller> + <controller type='pci' index='0' model='pci-root'/> + <input type='mouse' bus='ps2'/> + <input type='keyboard' bus='ps2'/> + <memballoon model='none'/> + </devices> +</domain> diff --git a/tests/qemuxml2xmloutdata/genid-active.xml b/tests/qemuxml2xmloutdata/genid-active.xml new file mode 100644 index 0000000000..fc41f2dd28 --- /dev/null +++ b/tests/qemuxml2xmloutdata/genid-active.xml @@ -0,0 +1,32 @@ +<domain type='qemu' id='1'> + <name>QEMUGuest1</name> + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> + <genid>e9392370-2917-565e-692b-d057f46512d6</genid> + <memory unit='KiB'>219136</memory> + <currentMemory unit='KiB'>219136</currentMemory> + <vcpu placement='static'>1</vcpu> + <os> + <type arch='x86_64' machine='pc'>hvm</type> + <boot dev='hd'/> + </os> + <features> + <acpi/> + </features> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash> + <devices> + <emulator>/usr/bin/qemu-system-x86_64</emulator> + <controller type='usb' index='0'> + <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x2'/> + </controller> + <controller type='ide' index='0'> + <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x1'/> + </controller> + <controller type='pci' index='0' model='pci-root'/> + <input type='mouse' bus='ps2'/> + <input type='keyboard' bus='ps2'/> + <memballoon model='none'/> + </devices> +</domain> diff --git a/tests/qemuxml2xmloutdata/genid-auto-active.xml b/tests/qemuxml2xmloutdata/genid-auto-active.xml new file mode 100644 index 0000000000..aeca0d7fc0 --- /dev/null +++ b/tests/qemuxml2xmloutdata/genid-auto-active.xml @@ -0,0 +1,32 @@ +<domain type='qemu' id='1'> + <name>QEMUGuest1</name> + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> + <genid>00010203-0405-4607-8809-0a0b0c0d0e0f</genid> + <memory unit='KiB'>219136</memory> + <currentMemory unit='KiB'>219136</currentMemory> + <vcpu placement='static'>1</vcpu> + <os> + <type arch='x86_64' machine='pc'>hvm</type> + <boot dev='hd'/> + </os> + <features> + <acpi/> + </features> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash> + <devices> + <emulator>/usr/bin/qemu-system-x86_64</emulator> + <controller type='usb' index='0'> + <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x2'/> + </controller> + <controller type='ide' index='0'> + <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x1'/> + </controller> + <controller type='pci' index='0' model='pci-root'/> + <input type='mouse' bus='ps2'/> + <input type='keyboard' bus='ps2'/> + <memballoon model='none'/> + </devices> +</domain> diff --git a/tests/qemuxml2xmloutdata/genid-auto-inactive.xml b/tests/qemuxml2xmloutdata/genid-auto-inactive.xml new file mode 100644 index 0000000000..83c924395b --- /dev/null +++ b/tests/qemuxml2xmloutdata/genid-auto-inactive.xml @@ -0,0 +1,32 @@ +<domain type='qemu'> + <name>QEMUGuest1</name> + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> + <genid/> + <memory unit='KiB'>219136</memory> + <currentMemory unit='KiB'>219136</currentMemory> + <vcpu placement='static'>1</vcpu> + <os> + <type arch='x86_64' machine='pc'>hvm</type> + <boot dev='hd'/> + </os> + <features> + <acpi/> + </features> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash> + <devices> + <emulator>/usr/bin/qemu-system-x86_64</emulator> + <controller type='usb' index='0'> + <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x2'/> + </controller> + <controller type='ide' index='0'> + <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x1'/> + </controller> + <controller type='pci' index='0' model='pci-root'/> + <input type='mouse' bus='ps2'/> + <input type='keyboard' bus='ps2'/> + <memballoon model='none'/> + </devices> +</domain> diff --git a/tests/qemuxml2xmloutdata/genid-inactive.xml b/tests/qemuxml2xmloutdata/genid-inactive.xml new file mode 100644 index 0000000000..8bd526a7a9 --- /dev/null +++ b/tests/qemuxml2xmloutdata/genid-inactive.xml @@ -0,0 +1,32 @@ +<domain type='qemu'> + <name>QEMUGuest1</name> + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> + <genid>e9392370-2917-565e-692b-d057f46512d6</genid> + <memory unit='KiB'>219136</memory> + <currentMemory unit='KiB'>219136</currentMemory> + <vcpu placement='static'>1</vcpu> + <os> + <type arch='x86_64' machine='pc'>hvm</type> + <boot dev='hd'/> + </os> + <features> + <acpi/> + </features> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash> + <devices> + <emulator>/usr/bin/qemu-system-x86_64</emulator> + <controller type='usb' index='0'> + <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x2'/> + </controller> + <controller type='ide' index='0'> + <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x1'/> + </controller> + <controller type='pci' index='0' model='pci-root'/> + <input type='mouse' bus='ps2'/> + <input type='keyboard' bus='ps2'/> + <memballoon model='none'/> + </devices> +</domain> diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c index 9e77b9fb13..e999810e12 100644 --- a/tests/qemuxml2xmltest.c +++ b/tests/qemuxml2xmltest.c @@ -276,6 +276,8 @@ mymain(void) setenv("PATH", "/bin", 1); DO_TEST("minimal", NONE); + DO_TEST("genid", NONE); + DO_TEST("genid-auto", NONE); DO_TEST("machine-core-on", NONE); DO_TEST("machine-core-off", NONE); DO_TEST("machine-loadparm-multiple-disks-nets-s390", NONE); @@ -1209,7 +1211,8 @@ mymain(void) } VIR_TEST_MAIN_PRELOAD(mymain, - abs_builddir "/.libs/virpcimock.so") + abs_builddir "/.libs/virpcimock.so", + abs_builddir "/.libs/virrandommock.so") #else -- 2.13.6
On Mon, Apr 23, 2018 at 20:00:00 -0400, John Ferlan wrote:
The VM Generation ID is a mechanism to provide a unique 128-bit, cryptographically random, and integer value identifier known as the GUID (Globally Unique Identifier) to the guest OS. The value is used to help notify the guest operating system when the virtual machine is executed with a different configuration.
This patch adds support for a new "genid" XML element similar to the "uuid" element. The "genid" element can have two forms "<genid/>" or "<genid>$GUID</genid>". If the $GUID is not provided, libvirt will generate one.
For the ABI checks add avoidance for the genid comparison if the appropriate flag is set.
Since adding support for a generated GUID (or UUID like) value to be displayed only for an active guest, modifying the xml2xml test to include virrandommock.so is necessary since it will generate a "known" UUID value that can be compared against for the active test.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- docs/formatdomain.html.in | 29 ++++++++++++ docs/schemas/domaincommon.rng | 8 ++++ src/conf/domain_conf.c | 59 ++++++++++++++++++++++++ src/conf/domain_conf.h | 3 ++ tests/qemuxml2argvdata/genid-auto.xml | 32 +++++++++++++ tests/qemuxml2argvdata/genid.xml | 32 +++++++++++++ tests/qemuxml2xmloutdata/genid-active.xml | 32 +++++++++++++ tests/qemuxml2xmloutdata/genid-auto-active.xml | 32 +++++++++++++ tests/qemuxml2xmloutdata/genid-auto-inactive.xml | 32 +++++++++++++ tests/qemuxml2xmloutdata/genid-inactive.xml | 32 +++++++++++++ tests/qemuxml2xmltest.c | 5 +- 11 files changed, 295 insertions(+), 1 deletion(-) create mode 100644 tests/qemuxml2argvdata/genid-auto.xml create mode 100644 tests/qemuxml2argvdata/genid.xml create mode 100644 tests/qemuxml2xmloutdata/genid-active.xml create mode 100644 tests/qemuxml2xmloutdata/genid-auto-active.xml create mode 100644 tests/qemuxml2xmloutdata/genid-auto-inactive.xml create mode 100644 tests/qemuxml2xmloutdata/genid-inactive.xml
diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index ada0df227f..6a140f3e40 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -34,6 +34,7 @@ <domain type='kvm' id='1'> <name>MyGuest</name> <uuid>4dea22b3-1d52-d8f3-2516-782e98ab3fa0</uuid> + <genid>43dc0cf8-809b-4adb-9bea-a9abb5f3d90e</genid>
Since we encourage to use the variant with this being empty I'd show that here.
<title>A short description - title - of the domain</title> <description>Some human readable description</description> <metadata> @@ -61,6 +62,34 @@ specification. <span class="since">Since 0.0.1, sysinfo since 0.8.7</span></dd>
+ <dt><code>genid</code></dt> + <dd><span class="since">Since 4.3.0</span>, the <code>genid</code> + element can be used to add a Virtual Machine Generation ID which + exposes a 128-bit, cryptographically random, integer value identifier, + referred to as a Globally Unique Identifier (GUID) using the same + format as the <code>uuid</code>. The value is used to help notify + the guest operating system when the virtual machine is executed + with a different configuration, such as: + + <ul> + <li>snapshot execution</li> + <li>backup recovery</li> + <li>failover in a disaster recovery environment</li> + <li>creation from template (import, copy, clone)</li> + </ul> + + The guest operating system notices the change and is then able to + react as appropriate by marking its copies of distributed databases + as dirty, re-initializing its random number generator, etc. + + <p> + When a GUID value is not provided, e.g. using the XML syntax + <genid/>, then libvirt will automatically generate a GUID. + This is the recommended configuration since the hypervisor then + can handle changing the GUID value for specific state transitions. + Using a static GUID value may result in failures for starting from + snapshot, restoring from backup, starting a cloned domain, etc.</p></dd> + <dt><code>title</code></dt> <dd>The optional element <code>title</code> provides space for a short description of the domain. The title should not contain
[...]
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 6d4db50998..8c30adf850 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -18793,6 +18793,34 @@ virDomainDefParseXML(xmlDocPtr xml, VIR_FREE(tmp); }
+ /* Extract domain genid - a genid can either be provided or generated */ + if ((n = virXPathNodeSet("./genid", ctxt, &nodes)) < 0) + goto error; + + if (n > 0) { + if (n != 1) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("element 'genid' can only appear once")); + goto error; + } + def->genidRequested = true; + if (!(tmp = virXPathString("string(./genid[1])", ctxt))) { + if (virUUIDGenerate(def->genid) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + "%s", _("Failed to generate genid"));
Generating this here is pointless, the code using it throws this away and generates a new one. While we don't show this value to the user and thus don't create any false impressions I think that the logic should be shuffled around so that it's generated only when it's required.
+ } + def->genidGenerated = true; + } else { + if (virUUIDParse(tmp, def->genid) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + "%s", _("malformed genid element")); + goto error; + } + } + } + VIR_FREE(nodes); + /* Extract short description of domain (title) */ def->title = virXPathString("string(./title[1])", ctxt); if (def->title && strchr(def->title, '\n')) { @@ -21904,6 +21932,26 @@ virDomainDefCheckABIStabilityFlags(virDomainDefPtr src, goto error; }
+ if (src->genidRequested != dst->genidRequested) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("Target domain requested genid does not match source")); + goto error; + } + + if (src->genidRequested && + !(flags & VIR_DOMAIN_DEF_ABI_CHECK_SKIP_GENID) &&
The above change will also possibly remove the need for this flag. In cases when it needs to change (snapshots) the code is skipping the check anyways. In cases when it needs to stay the same we will know that since it will be filled in.
+ memcmp(src->genid, dst->genid, VIR_UUID_BUFLEN) != 0) { + char guidsrc[VIR_UUID_STRING_BUFLEN]; + char guiddst[VIR_UUID_STRING_BUFLEN]; + + virUUIDFormat(src->genid, guidsrc); + virUUIDFormat(dst->genid, guiddst); + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("Target domain genid %s does not match source %s"), + guiddst, guidsrc); + goto error; + } + /* Not strictly ABI related, but we want to make sure domains * don't get silently re-named through the backdoor when passing * custom XML into various APIs, since this would create havoc
On 04/24/2018 03:21 AM, Peter Krempa wrote:
On Mon, Apr 23, 2018 at 20:00:00 -0400, John Ferlan wrote:
The VM Generation ID is a mechanism to provide a unique 128-bit, cryptographically random, and integer value identifier known as the GUID (Globally Unique Identifier) to the guest OS. The value is used to help notify the guest operating system when the virtual machine is executed with a different configuration.
This patch adds support for a new "genid" XML element similar to the "uuid" element. The "genid" element can have two forms "<genid/>" or "<genid>$GUID</genid>". If the $GUID is not provided, libvirt will generate one.
For the ABI checks add avoidance for the genid comparison if the appropriate flag is set.
Since adding support for a generated GUID (or UUID like) value to be displayed only for an active guest, modifying the xml2xml test to include virrandommock.so is necessary since it will generate a "known" UUID value that can be compared against for the active test.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- docs/formatdomain.html.in | 29 ++++++++++++ docs/schemas/domaincommon.rng | 8 ++++ src/conf/domain_conf.c | 59 ++++++++++++++++++++++++ src/conf/domain_conf.h | 3 ++ tests/qemuxml2argvdata/genid-auto.xml | 32 +++++++++++++ tests/qemuxml2argvdata/genid.xml | 32 +++++++++++++ tests/qemuxml2xmloutdata/genid-active.xml | 32 +++++++++++++ tests/qemuxml2xmloutdata/genid-auto-active.xml | 32 +++++++++++++ tests/qemuxml2xmloutdata/genid-auto-inactive.xml | 32 +++++++++++++ tests/qemuxml2xmloutdata/genid-inactive.xml | 32 +++++++++++++ tests/qemuxml2xmltest.c | 5 +- 11 files changed, 295 insertions(+), 1 deletion(-) create mode 100644 tests/qemuxml2argvdata/genid-auto.xml create mode 100644 tests/qemuxml2argvdata/genid.xml create mode 100644 tests/qemuxml2xmloutdata/genid-active.xml create mode 100644 tests/qemuxml2xmloutdata/genid-auto-active.xml create mode 100644 tests/qemuxml2xmloutdata/genid-auto-inactive.xml create mode 100644 tests/qemuxml2xmloutdata/genid-inactive.xml
diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index ada0df227f..6a140f3e40 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -34,6 +34,7 @@ <domain type='kvm' id='1'> <name>MyGuest</name> <uuid>4dea22b3-1d52-d8f3-2516-782e98ab3fa0</uuid> + <genid>43dc0cf8-809b-4adb-9bea-a9abb5f3d90e</genid>
Since we encourage to use the variant with this being empty I'd show that here.
I'd agree except for the fact this is a "live" XML example since "id='1'", so it should stay as is unless of course it's desired to never show the GUID for the running VM.
<title>A short description - title - of the domain</title> <description>Some human readable description</description> <metadata> @@ -61,6 +62,34 @@ specification. <span class="since">Since 0.0.1, sysinfo since 0.8.7</span></dd>
+ <dt><code>genid</code></dt> + <dd><span class="since">Since 4.3.0</span>, the <code>genid</code> + element can be used to add a Virtual Machine Generation ID which + exposes a 128-bit, cryptographically random, integer value identifier, + referred to as a Globally Unique Identifier (GUID) using the same + format as the <code>uuid</code>. The value is used to help notify + the guest operating system when the virtual machine is executed + with a different configuration, such as: + + <ul> + <li>snapshot execution</li> + <li>backup recovery</li> + <li>failover in a disaster recovery environment</li> + <li>creation from template (import, copy, clone)</li> + </ul> + + The guest operating system notices the change and is then able to + react as appropriate by marking its copies of distributed databases + as dirty, re-initializing its random number generator, etc. + + <p> + When a GUID value is not provided, e.g. using the XML syntax + <genid/>, then libvirt will automatically generate a GUID. + This is the recommended configuration since the hypervisor then + can handle changing the GUID value for specific state transitions. + Using a static GUID value may result in failures for starting from + snapshot, restoring from backup, starting a cloned domain, etc.</p></dd> + <dt><code>title</code></dt> <dd>The optional element <code>title</code> provides space for a short description of the domain. The title should not contain
[...]
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 6d4db50998..8c30adf850 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -18793,6 +18793,34 @@ virDomainDefParseXML(xmlDocPtr xml, VIR_FREE(tmp); }
+ /* Extract domain genid - a genid can either be provided or generated */ + if ((n = virXPathNodeSet("./genid", ctxt, &nodes)) < 0) + goto error; + + if (n > 0) { + if (n != 1) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("element 'genid' can only appear once")); + goto error; + } + def->genidRequested = true; + if (!(tmp = virXPathString("string(./genid[1])", ctxt))) { + if (virUUIDGenerate(def->genid) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + "%s", _("Failed to generate genid"));
Generating this here is pointless, the code using it throws this away and generates a new one. While we don't show this value to the user and thus don't create any false impressions I think that the logic should be shuffled around so that it's generated only when it's required.
How so? Not generating one here would cause active xml-2-xml to fail (as I expect) because the GUID would be 00000-0000-0000-0000-000000000000. At the end of the series the usage of virUUIDGenerate for ->genid is done for: 1. This code, virDomainDefParseXML 2. virDomainDefCopy when genidRequested and flags & VIR_DOMAIN_DEF_COPY_NEWGENID which is currently only used by the snapshot code, but could be used elsewhere - something I may have been thinking about at least w/r/t one of the qemu_driver paths. 3. qemuProcessGenID *when* flags & VIR_QEMU_PROCESS_START_GEN_VMID If anything, perhaps what you may be referring to is qemuProcessGenID processing using the VIR_QEMU_PROCESS_START_GEN_VMID flag when called from qemuDomainRevertToSnapshot. The transitions there caused me to be "super cautious", but the Copy, then Start I think upon reflection does overwrite. The same flag is used in qemuDomainSaveImageStartVM, which yes does overwrite, but that's by rule/design. Perhaps it'll help to summarize the transitions... Change is not required for the following transitions: -> Pause/Resume -> VM Reboot -> Host reboot -> Live migrate -> Failover in a clustered environment Change is required for the following transitions: -> Application of offline/online snapshot -> Restoring from backup -> Failover to replication target -> Importing a VM (or copy or clone) Change is "Unspecified" when a VM's configuration changes. So the 'design decisions' were: - For snapshot, the choice was use the virDomainDefCopy and ABI flags. - For the restore from backup, the choice was regenerate and store in the config during startup processing. The docs indicate "The restore sequence will be modified to post process the restore target and apply new generation identifiers to the restored configuration files." - so where it was done would - It's not 100% clear if we need something special for failover from replication target. Seems like Snapshot or Save/Restore is in the same category, but perhaps there's some hypervisor specific transition. - For import and copy, changing the genid of the copy "seems right". The tricky part is the clone code which would require a separate change to virt-clone since it uses parses and formats XML in separate passes. So given all that - are you still of the opinion that the design needs to change even more? If so, then please also characterize your view of how this should work. Tks - John
+ } + def->genidGenerated = true; + } else { + if (virUUIDParse(tmp, def->genid) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + "%s", _("malformed genid element")); + goto error; + } + } + } + VIR_FREE(nodes); + /* Extract short description of domain (title) */ def->title = virXPathString("string(./title[1])", ctxt); if (def->title && strchr(def->title, '\n')) { @@ -21904,6 +21932,26 @@ virDomainDefCheckABIStabilityFlags(virDomainDefPtr src, goto error; }
+ if (src->genidRequested != dst->genidRequested) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("Target domain requested genid does not match source")); + goto error; + } + + if (src->genidRequested && + !(flags & VIR_DOMAIN_DEF_ABI_CHECK_SKIP_GENID) &&
The above change will also possibly remove the need for this flag. In cases when it needs to change (snapshots) the code is skipping the check anyways. In cases when it needs to stay the same we will know that since it will be filled in.
+ memcmp(src->genid, dst->genid, VIR_UUID_BUFLEN) != 0) { + char guidsrc[VIR_UUID_STRING_BUFLEN]; + char guiddst[VIR_UUID_STRING_BUFLEN]; + + virUUIDFormat(src->genid, guidsrc); + virUUIDFormat(dst->genid, guiddst); + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("Target domain genid %s does not match source %s"), + guiddst, guidsrc); + goto error; + } + /* Not strictly ABI related, but we want to make sure domains * don't get silently re-named through the backdoor when passing * custom XML into various APIs, since this would create havoc
On Tue, Apr 24, 2018 at 15:38:40 -0400, John Ferlan wrote:
On 04/24/2018 03:21 AM, Peter Krempa wrote:
On Mon, Apr 23, 2018 at 20:00:00 -0400, John Ferlan wrote:
The VM Generation ID is a mechanism to provide a unique 128-bit, cryptographically random, and integer value identifier known as the GUID (Globally Unique Identifier) to the guest OS. The value is used to help notify the guest operating system when the virtual machine is executed with a different configuration.
This patch adds support for a new "genid" XML element similar to the "uuid" element. The "genid" element can have two forms "<genid/>" or "<genid>$GUID</genid>". If the $GUID is not provided, libvirt will generate one.
For the ABI checks add avoidance for the genid comparison if the appropriate flag is set.
Since adding support for a generated GUID (or UUID like) value to be displayed only for an active guest, modifying the xml2xml test to include virrandommock.so is necessary since it will generate a "known" UUID value that can be compared against for the active test.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- docs/formatdomain.html.in | 29 ++++++++++++ docs/schemas/domaincommon.rng | 8 ++++ src/conf/domain_conf.c | 59 ++++++++++++++++++++++++ src/conf/domain_conf.h | 3 ++ tests/qemuxml2argvdata/genid-auto.xml | 32 +++++++++++++ tests/qemuxml2argvdata/genid.xml | 32 +++++++++++++ tests/qemuxml2xmloutdata/genid-active.xml | 32 +++++++++++++ tests/qemuxml2xmloutdata/genid-auto-active.xml | 32 +++++++++++++ tests/qemuxml2xmloutdata/genid-auto-inactive.xml | 32 +++++++++++++ tests/qemuxml2xmloutdata/genid-inactive.xml | 32 +++++++++++++ tests/qemuxml2xmltest.c | 5 +- 11 files changed, 295 insertions(+), 1 deletion(-) create mode 100644 tests/qemuxml2argvdata/genid-auto.xml create mode 100644 tests/qemuxml2argvdata/genid.xml create mode 100644 tests/qemuxml2xmloutdata/genid-active.xml create mode 100644 tests/qemuxml2xmloutdata/genid-auto-active.xml create mode 100644 tests/qemuxml2xmloutdata/genid-auto-inactive.xml create mode 100644 tests/qemuxml2xmloutdata/genid-inactive.xml
diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index ada0df227f..6a140f3e40 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -34,6 +34,7 @@ <domain type='kvm' id='1'> <name>MyGuest</name> <uuid>4dea22b3-1d52-d8f3-2516-782e98ab3fa0</uuid> + <genid>43dc0cf8-809b-4adb-9bea-a9abb5f3d90e</genid>
Since we encourage to use the variant with this being empty I'd show that here.
I'd agree except for the fact this is a "live" XML example since "id='1'", so it should stay as is unless of course it's desired to never show the GUID for the running VM.
Hmm, right. It feels slightly wrong though that we are describing configuration on the example of a live XML.
<title>A short description - title - of the domain</title> <description>Some human readable description</description> <metadata>
[...]
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 6d4db50998..8c30adf850 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -18793,6 +18793,34 @@ virDomainDefParseXML(xmlDocPtr xml, VIR_FREE(tmp); }
+ /* Extract domain genid - a genid can either be provided or generated */ + if ((n = virXPathNodeSet("./genid", ctxt, &nodes)) < 0) + goto error; + + if (n > 0) { + if (n != 1) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("element 'genid' can only appear once")); + goto error; + } + def->genidRequested = true; + if (!(tmp = virXPathString("string(./genid[1])", ctxt))) { + if (virUUIDGenerate(def->genid) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + "%s", _("Failed to generate genid"));
Generating this here is pointless, the code using it throws this away and generates a new one. While we don't show this value to the user and thus don't create any false impressions I think that the logic should be shuffled around so that it's generated only when it's required.
How so? Not generating one here would cause active xml-2-xml to fail (as
Basically because a GUID for an inactive definition is nonsense. It will necessarily change upon startup, so having it generated in the parser is pointless. The parser only needs to restore a user-provided GUID or the one which is store
I expect) because the GUID would be 00000-0000-0000-0000-000000000000. At the end of the series the usage of virUUIDGenerate for ->genid is done for:
1. This code, virDomainDefParseXML 2. virDomainDefCopy when genidRequested and flags & VIR_DOMAIN_DEF_COPY_NEWGENID which is currently only used by the snapshot code, but could be used elsewhere - something I may have been thinking about at least w/r/t one of the qemu_driver paths. 3. qemuProcessGenID *when* flags & VIR_QEMU_PROCESS_START_GEN_VMID
If anything, perhaps what you may be referring to is qemuProcessGenID processing using the VIR_QEMU_PROCESS_START_GEN_VMID flag when called from qemuDomainRevertToSnapshot. The transitions there caused me to be "super cautious", but the Copy, then Start I think upon reflection does overwrite. The same flag is used in qemuDomainSaveImageStartVM, which yes does overwrite, but that's by rule/design.
Perhaps it'll help to summarize the transitions...
Change is not required for the following transitions:
-> Pause/Resume -> VM Reboot
Both above are the same emulator instance.
-> Host reboot
This is wrong. Host reboot causes the VM to be killed. So when booting the GUID will change since we start a new process. While it should not result in any problems if the ID would not change (since the guest OS rebooted anyways) we should treat this as start of the VM.
-> Live migrate
This is technically the same emulator instance. No rollback of execution was possible.
-> Failover in a clustered environment
Thankfully it does not apply at this point since we don't support any of this implicitly. It also probably depends on the way the failover scenario is executed. In the qemu world I read only about coarse-grained lockstepping as a case of failover and that has situations where some rollback could happen, so in that case the VM id should probably change. But as said, thankfully we don't have to deal with it currenly and also it would not be possible since qemu can't change the id during runtime.
Change is required for the following transitions:
-> Application of offline/online snapshot -> Restoring from backup
This is too vague. If you mean a disk backup, the VM will boot, thus the ID should change (unless the user specified an explicit one).
-> Failover to replication target
See above.
-> Importing a VM (or copy or clone)
Change is "Unspecified" when a VM's configuration changes.
So in our case it will change.
So the 'design decisions' were:
- For snapshot, the choice was use the virDomainDefCopy and ABI flags.
Actually, the GUID when creating the snapshot is irrelevant. When reverting a snapshot the ID should always change thus it will always require an emulator restart.
- For the restore from backup, the choice was regenerate and store in the config during startup processing. The docs indicate "The restore sequence will be modified to post process the restore target and apply new generation identifiers to the restored configuration files." - so where it was done would
As pointed out above, this is vague. If it's a restore of the disk state only, the guid will necessarily change in our design when we start the new qemu process.
- It's not 100% clear if we need something special for failover from replication target. Seems like Snapshot or Save/Restore is in the same category, but perhaps there's some hypervisor specific transition.
What is a failover here? You mean if a management starts a new VM after a different host with VM has failed? In such case it's a new start of VM for us and thus will get a new GUID.
- For import and copy, changing the genid of the copy "seems right". The tricky part is the clone code which would require a separate change to virt-clone since it uses parses and formats XML in separate passes.
That depends on the implementation. Currently yes.
So given all that - are you still of the opinion that the design needs to change even more? If so, then please also characterize your view of how this should work.
Well, that depends. I did not read the docs for this thoroughly enough. If it is okay for us to generate a new GUID upon every boot of a VM then this will be for a rather simple implementation, since we have a very limited set of situations when we are starting a new qemu process which should NOT change the GUID and we will change it in all other scenarios. If the documentation does not allow for the above it will be more complex and we'll need to discuss that further. A second consideration then is whether to allow user-specified GUID at all, but I guess mgmt applications may have a different idea when it's necessary to change and thus it makes sense to allow that. For that situation the provided GUID should be always honoured. This means that we'll probably need a new attribute which will specify that the GUID is custom (or the opposite, that it was generated). If that property is in the default state the startup procedure should generate a new GUID exept for the migration case. We also might want to consider the managed-save case to bear the same GUID after startup, since we know that we start the new qemu process from the same state as we've saved it.
On Wed, Apr 25, 2018 at 10:32:05AM +0200, Peter Krempa wrote:
Well, that depends. I did not read the docs for this thoroughly enough. If it is okay for us to generate a new GUID upon every boot of a VM then this will be for a rather simple implementation, since we have a very limited set of situations when we are starting a new qemu process which should NOT change the GUID and we will change it in all other scenarios.
AFAIK, we *must* change GUID on every cold boot
A second consideration then is whether to allow user-specified GUID at all, but I guess mgmt applications may have a different idea when it's necessary to change and thus it makes sense to allow that. For that situation the provided GUID should be always honoured.
The microsoft spec describes exactly why GUID must change, and mgmt applications must not deviate from those rules to make up their own. So the question is not whether the mgmt app has a different idea of when to change GUID. Rather, we should consider whether there is any situation in which it is impossible for libvirt todo the right thing according to the microsoft spec. If libvirt has enough knowledge to always do the right thing, then we could refuse to make it user configurable - just report what is set. Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
On Wed, Apr 25, 2018 at 09:39:49 +0100, Daniel Berrange wrote:
On Wed, Apr 25, 2018 at 10:32:05AM +0200, Peter Krempa wrote:
Well, that depends. I did not read the docs for this thoroughly enough. If it is okay for us to generate a new GUID upon every boot of a VM then this will be for a rather simple implementation, since we have a very limited set of situations when we are starting a new qemu process which should NOT change the GUID and we will change it in all other scenarios.
AFAIK, we *must* change GUID on every cold boot
Good, that makes things rather simple.
A second consideration then is whether to allow user-specified GUID at all, but I guess mgmt applications may have a different idea when it's necessary to change and thus it makes sense to allow that. For that situation the provided GUID should be always honoured.
The microsoft spec describes exactly why GUID must change, and mgmt applications must not deviate from those rules to make up their own.
So the question is not whether the mgmt app has a different idea of when to change GUID. Rather, we should consider whether there is any situation in which it is impossible for libvirt todo the right thing according to the microsoft spec.
If libvirt has enough knowledge to always do the right thing, then we could refuse to make it user configurable - just report what is set.
The only thing that comes into my mind is the non-managed save/restore case. In that scenario both options make somewhat sense and libvirt can't really tell which is the case. That can be handled with an API flag though, since we can use the GUID stored in the save image.
On 04/25/2018 04:46 AM, Peter Krempa wrote:
On Wed, Apr 25, 2018 at 09:39:49 +0100, Daniel Berrange wrote:
On Wed, Apr 25, 2018 at 10:32:05AM +0200, Peter Krempa wrote:
Well, that depends. I did not read the docs for this thoroughly enough. If it is okay for us to generate a new GUID upon every boot of a VM then this will be for a rather simple implementation, since we have a very limited set of situations when we are starting a new qemu process which should NOT change the GUID and we will change it in all other scenarios.
AFAIK, we *must* change GUID on every cold boot
Good, that makes things rather simple.
This one is not really 100% clear to me. The "spec" is like 6 pages - it's a pretty quick read... http://go.microsoft.com/fwlink/?LinkId=260709 The last 2 pages describe "when" the GUID should change and specifically calling out cold boot is not one of those. What leads me to believe otherwise is the two boot scenarios and the unspecified VM config changes have this "undertone" that using the same GUID for those scenarios is fine/expected. I really don't want to follow the ~20 attempts to get genid into QEMU - I'll give up way before then ;-)!
A second consideration then is whether to allow user-specified GUID at all, but I guess mgmt applications may have a different idea when it's necessary to change and thus it makes sense to allow that. For that situation the provided GUID should be always honoured.
The microsoft spec describes exactly why GUID must change, and mgmt applications must not deviate from those rules to make up their own.
So the question is not whether the mgmt app has a different idea of when to change GUID. Rather, we should consider whether there is any situation in which it is impossible for libvirt todo the right thing according to the microsoft spec.
If libvirt has enough knowledge to always do the right thing, then we could refuse to make it user configurable - just report what is set.
Not sure we can "decide" to not make it user configurable, but then again it's also not specifically stated from how I read things.
The only thing that comes into my mind is the non-managed save/restore case. In that scenario both options make somewhat sense and libvirt can't really tell which is the case.
That can be handled with an API flag though, since we can use the GUID stored in the save image.
The spec says : "Restoring from backup – Restoring a backup image will cause a previous workload time interval to be re-executed. Upon restore, the components of the backup are enumerated and replaced on the restore target by the VSS system. The affected configuration files are simply copied and not re-realized on the host. The restore sequence will be modified to post process the restore target and apply new generation identifiers to the restored configuration files." I read that as change the GUID and did so during startup processing from the qemuDomainSaveImageStartVM. Although I suppose it could also be read as altering the GUID before the virDomainObjListAdd in qemuDomainRestoreFlags and before the virDomainObjAssignDef in qemuDomainObjRestore. Since both called the latter and the running config for which the GUID is formatted is saved in the latter it "felt reasonable" to not have duplicated code. John
On Wed, Apr 25, 2018 at 08:02:35AM -0400, John Ferlan wrote:
On 04/25/2018 04:46 AM, Peter Krempa wrote:
On Wed, Apr 25, 2018 at 09:39:49 +0100, Daniel Berrange wrote:
On Wed, Apr 25, 2018 at 10:32:05AM +0200, Peter Krempa wrote:
Well, that depends. I did not read the docs for this thoroughly enough. If it is okay for us to generate a new GUID upon every boot of a VM then this will be for a rather simple implementation, since we have a very limited set of situations when we are starting a new qemu process which should NOT change the GUID and we will change it in all other scenarios.
AFAIK, we *must* change GUID on every cold boot
Good, that makes things rather simple.
This one is not really 100% clear to me. The "spec" is like 6 pages - it's a pretty quick read...
http://go.microsoft.com/fwlink/?LinkId=260709
The last 2 pages describe "when" the GUID should change and specifically calling out cold boot is not one of those. What leads me to believe otherwise is the two boot scenarios and the unspecified VM config changes have this "undertone" that using the same GUID for those scenarios is fine/expected.
Yeah the table at the very end is the key bit and it seems I was actually wrong Scenario GenID changed ----------------------------------------------------------------------- Virtual machine is paused or resumed No Virtual machine reboots No Virtual machine host reboots No Virtual machine starts executing a snapshot Yes Virtual machine is recovered from backup Yes Virtual machine is failed over in a disaster recovery env Yes Virtual machine is live migrated No Virtual machine is imported, copied, or cloned Yes Virtual machine is failed over in a clustered environment No Virtual machine's configuration changes Unspecified My reading of "Virtual machine reboots" and "Virtual machine host reboots" rows in particular is that we can *NOT* change GUID on every boot up operation. The spec literally only wants it to be changed when there is the possibility that the VM is potentially re-executing something that has already been executed before. The transient VM feature is the real killer for libvirt - we have no way of knowing when virDomainCreateXML launches a transient VM whether that is starting up after a revert to backup/snapshot, or whether it is a normal boot. Only the mgmt app like oVirt / OpenStack has enough info to decide that. So we must allow the mgmt app to provide a GUID in the XML document themselves, and then change it in places where we know it is needed to change. Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
On Wed, Apr 25, 2018 at 13:25:57 +0100, Daniel Berrange wrote:
On Wed, Apr 25, 2018 at 08:02:35AM -0400, John Ferlan wrote:
On 04/25/2018 04:46 AM, Peter Krempa wrote:
On Wed, Apr 25, 2018 at 09:39:49 +0100, Daniel Berrange wrote:
On Wed, Apr 25, 2018 at 10:32:05AM +0200, Peter Krempa wrote:
Well, that depends. I did not read the docs for this thoroughly enough. If it is okay for us to generate a new GUID upon every boot of a VM then this will be for a rather simple implementation, since we have a very limited set of situations when we are starting a new qemu process which should NOT change the GUID and we will change it in all other scenarios.
AFAIK, we *must* change GUID on every cold boot
Good, that makes things rather simple.
This one is not really 100% clear to me. The "spec" is like 6 pages - it's a pretty quick read...
http://go.microsoft.com/fwlink/?LinkId=260709
The last 2 pages describe "when" the GUID should change and specifically calling out cold boot is not one of those. What leads me to believe otherwise is the two boot scenarios and the unspecified VM config changes have this "undertone" that using the same GUID for those scenarios is fine/expected.
Yeah the table at the very end is the key bit and it seems I was actually wrong
Scenario GenID changed ----------------------------------------------------------------------- Virtual machine is paused or resumed No Virtual machine reboots No Virtual machine host reboots No Virtual machine starts executing a snapshot Yes Virtual machine is recovered from backup Yes Virtual machine is failed over in a disaster recovery env Yes Virtual machine is live migrated No Virtual machine is imported, copied, or cloned Yes Virtual machine is failed over in a clustered environment No Virtual machine's configuration changes Unspecified
My reading of "Virtual machine reboots" and "Virtual machine host reboots" rows in particular is that we can *NOT* change GUID on every boot up operation. The spec literally only wants it to be changed when there is the possibility that the VM is potentially re-executing something that has already been executed before.
The transient VM feature is the real killer for libvirt - we have no way of knowing when virDomainCreateXML launches a transient VM whether that is starting up after a revert to backup/snapshot, or whether it is a normal boot. Only the mgmt app like oVirt / OpenStack has enough info to decide that. So we must allow the mgmt app to provide a GUID in the XML document themselves, and then change it in places where we know it is needed to change.
Okay. So that means that we actually need to generate it in the parser, but we should also always report it back even for offline configurations. The only problem then will be what to do with the save/restore functionality, because that is really unknown to us since that API both includes the "Virtual machine is paused or resumed" and "Virtual machine starts executing a snapshot" scenario.
On Wed, Apr 25, 2018 at 02:36:52PM +0200, Peter Krempa wrote:
On Wed, Apr 25, 2018 at 13:25:57 +0100, Daniel Berrange wrote:
On Wed, Apr 25, 2018 at 08:02:35AM -0400, John Ferlan wrote:
On 04/25/2018 04:46 AM, Peter Krempa wrote:
On Wed, Apr 25, 2018 at 09:39:49 +0100, Daniel Berrange wrote:
On Wed, Apr 25, 2018 at 10:32:05AM +0200, Peter Krempa wrote:
Well, that depends. I did not read the docs for this thoroughly enough. If it is okay for us to generate a new GUID upon every boot of a VM then this will be for a rather simple implementation, since we have a very limited set of situations when we are starting a new qemu process which should NOT change the GUID and we will change it in all other scenarios.
AFAIK, we *must* change GUID on every cold boot
Good, that makes things rather simple.
This one is not really 100% clear to me. The "spec" is like 6 pages - it's a pretty quick read...
http://go.microsoft.com/fwlink/?LinkId=260709
The last 2 pages describe "when" the GUID should change and specifically calling out cold boot is not one of those. What leads me to believe otherwise is the two boot scenarios and the unspecified VM config changes have this "undertone" that using the same GUID for those scenarios is fine/expected.
Yeah the table at the very end is the key bit and it seems I was actually wrong
Scenario GenID changed ----------------------------------------------------------------------- Virtual machine is paused or resumed No Virtual machine reboots No Virtual machine host reboots No Virtual machine starts executing a snapshot Yes Virtual machine is recovered from backup Yes Virtual machine is failed over in a disaster recovery env Yes Virtual machine is live migrated No Virtual machine is imported, copied, or cloned Yes Virtual machine is failed over in a clustered environment No Virtual machine's configuration changes Unspecified
My reading of "Virtual machine reboots" and "Virtual machine host reboots" rows in particular is that we can *NOT* change GUID on every boot up operation. The spec literally only wants it to be changed when there is the possibility that the VM is potentially re-executing something that has already been executed before.
The transient VM feature is the real killer for libvirt - we have no way of knowing when virDomainCreateXML launches a transient VM whether that is starting up after a revert to backup/snapshot, or whether it is a normal boot. Only the mgmt app like oVirt / OpenStack has enough info to decide that. So we must allow the mgmt app to provide a GUID in the XML document themselves, and then change it in places where we know it is needed to change.
Okay. So that means that we actually need to generate it in the parser, but we should also always report it back even for offline configurations.
The only problem then will be what to do with the save/restore functionality, because that is really unknown to us since that API both includes the "Virtual machine is paused or resumed" and "Virtual machine starts executing a snapshot" scenario.
I think it would fall under the "starts executing a snapshot" scenario no matter what, because the spec doesn't say anything about whether the original VM carried on running after the snapshot was taken. Just that a snapshot was taken and this snapshot is then run. So the fact that our save/restore can be view as a way to "pause" execution doesn't matter - we've implemented "pause" by creating a snapshot and then "resume" by running the snapshot. Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
On 04/25/2018 08:41 AM, Daniel P. Berrangé wrote:
On Wed, Apr 25, 2018 at 02:36:52PM +0200, Peter Krempa wrote:
On Wed, Apr 25, 2018 at 13:25:57 +0100, Daniel Berrange wrote:
On Wed, Apr 25, 2018 at 08:02:35AM -0400, John Ferlan wrote:
On 04/25/2018 04:46 AM, Peter Krempa wrote:
On Wed, Apr 25, 2018 at 09:39:49 +0100, Daniel Berrange wrote:
On Wed, Apr 25, 2018 at 10:32:05AM +0200, Peter Krempa wrote: > > Well, that depends. I did not read the docs for this thoroughly enough. > If it is okay for us to generate a new GUID upon every boot of a VM then > this will be for a rather simple implementation, since we have a very > limited set of situations when we are starting a new qemu process which > should NOT change the GUID and we will change it in all other scenarios.
AFAIK, we *must* change GUID on every cold boot
Good, that makes things rather simple.
This one is not really 100% clear to me. The "spec" is like 6 pages - it's a pretty quick read...
http://go.microsoft.com/fwlink/?LinkId=260709
The last 2 pages describe "when" the GUID should change and specifically calling out cold boot is not one of those. What leads me to believe otherwise is the two boot scenarios and the unspecified VM config changes have this "undertone" that using the same GUID for those scenarios is fine/expected.
Yeah the table at the very end is the key bit and it seems I was actually wrong
Scenario GenID changed ----------------------------------------------------------------------- Virtual machine is paused or resumed No Virtual machine reboots No Virtual machine host reboots No Virtual machine starts executing a snapshot Yes Virtual machine is recovered from backup Yes Virtual machine is failed over in a disaster recovery env Yes Virtual machine is live migrated No Virtual machine is imported, copied, or cloned Yes Virtual machine is failed over in a clustered environment No Virtual machine's configuration changes Unspecified
My reading of "Virtual machine reboots" and "Virtual machine host reboots" rows in particular is that we can *NOT* change GUID on every boot up operation. The spec literally only wants it to be changed when there is the possibility that the VM is potentially re-executing something that has already been executed before.
The transient VM feature is the real killer for libvirt - we have no way of knowing when virDomainCreateXML launches a transient VM whether that is starting up after a revert to backup/snapshot, or whether it is a normal boot. Only the mgmt app like oVirt / OpenStack has enough info to decide that. So we must allow the mgmt app to provide a GUID in the XML document themselves, and then change it in places where we know it is needed to change.
Also allows virt-clone to adjust it too...
Okay. So that means that we actually need to generate it in the parser, but we should also always report it back even for offline configurations.
The only problem then will be what to do with the save/restore functionality, because that is really unknown to us since that API both includes the "Virtual machine is paused or resumed" and "Virtual machine starts executing a snapshot" scenario.
I think it would fall under the "starts executing a snapshot" scenario no matter what, because the spec doesn't say anything about whether the original VM carried on running after the snapshot was taken. Just that a snapshot was taken and this snapshot is then run. So the fact that our save/restore can be view as a way to "pause" execution doesn't matter - we've implemented "pause" by creating a snapshot and then "resume" by running the snapshot.
So given all this - beyond not altering virDomainDefCopy to add a new flag and removing the ABI flag since it was only put there because of RevertToSnapshot @config copy difference, does just altering the genid via the START_GEN_VMID flag then cover things? Is there some other transition that needs that? IOW: Drop patches 2, 3 & 5... Merge patch 4 & 6 and alter 8/9 to not worry about abiflags. Do we print the GUID on inactive domains where we generate? I would think it wouldn't matter and the answer should be no. The print would only happen when active, but it's not that important. Tks - John and yes, 4.3.0 is out of the question here - it'd be in 4.4.0 at the earliest.
On Wed, Apr 25, 2018 at 09:23:59AM -0400, John Ferlan wrote:
On 04/25/2018 08:41 AM, Daniel P. Berrangé wrote:
On Wed, Apr 25, 2018 at 02:36:52PM +0200, Peter Krempa wrote:
On Wed, Apr 25, 2018 at 13:25:57 +0100, Daniel Berrange wrote:
On Wed, Apr 25, 2018 at 08:02:35AM -0400, John Ferlan wrote:
On 04/25/2018 04:46 AM, Peter Krempa wrote:
On Wed, Apr 25, 2018 at 09:39:49 +0100, Daniel Berrange wrote: > On Wed, Apr 25, 2018 at 10:32:05AM +0200, Peter Krempa wrote: >> >> Well, that depends. I did not read the docs for this thoroughly enough. >> If it is okay for us to generate a new GUID upon every boot of a VM then >> this will be for a rather simple implementation, since we have a very >> limited set of situations when we are starting a new qemu process which >> should NOT change the GUID and we will change it in all other scenarios. > > AFAIK, we *must* change GUID on every cold boot
Good, that makes things rather simple.
This one is not really 100% clear to me. The "spec" is like 6 pages - it's a pretty quick read...
http://go.microsoft.com/fwlink/?LinkId=260709
The last 2 pages describe "when" the GUID should change and specifically calling out cold boot is not one of those. What leads me to believe otherwise is the two boot scenarios and the unspecified VM config changes have this "undertone" that using the same GUID for those scenarios is fine/expected.
Yeah the table at the very end is the key bit and it seems I was actually wrong
Scenario GenID changed ----------------------------------------------------------------------- Virtual machine is paused or resumed No Virtual machine reboots No Virtual machine host reboots No Virtual machine starts executing a snapshot Yes Virtual machine is recovered from backup Yes Virtual machine is failed over in a disaster recovery env Yes Virtual machine is live migrated No Virtual machine is imported, copied, or cloned Yes Virtual machine is failed over in a clustered environment No Virtual machine's configuration changes Unspecified
My reading of "Virtual machine reboots" and "Virtual machine host reboots" rows in particular is that we can *NOT* change GUID on every boot up operation. The spec literally only wants it to be changed when there is the possibility that the VM is potentially re-executing something that has already been executed before.
The transient VM feature is the real killer for libvirt - we have no way of knowing when virDomainCreateXML launches a transient VM whether that is starting up after a revert to backup/snapshot, or whether it is a normal boot. Only the mgmt app like oVirt / OpenStack has enough info to decide that. So we must allow the mgmt app to provide a GUID in the XML document themselves, and then change it in places where we know it is needed to change.
Also allows virt-clone to adjust it too...
Okay. So that means that we actually need to generate it in the parser, but we should also always report it back even for offline configurations.
The only problem then will be what to do with the save/restore functionality, because that is really unknown to us since that API both includes the "Virtual machine is paused or resumed" and "Virtual machine starts executing a snapshot" scenario.
I think it would fall under the "starts executing a snapshot" scenario no matter what, because the spec doesn't say anything about whether the original VM carried on running after the snapshot was taken. Just that a snapshot was taken and this snapshot is then run. So the fact that our save/restore can be view as a way to "pause" execution doesn't matter - we've implemented "pause" by creating a snapshot and then "resume" by running the snapshot.
So given all this - beyond not altering virDomainDefCopy to add a new flag and removing the ABI flag since it was only put there because of RevertToSnapshot @config copy difference, does just altering the genid via the START_GEN_VMID flag then cover things? Is there some other transition that needs that?
IOW: Drop patches 2, 3 & 5... Merge patch 4 & 6 and alter 8/9 to not worry about abiflags.
Do we print the GUID on inactive domains where we generate? I would think it wouldn't matter and the answer should be no. The print would only happen when active, but it's not that important.
Once we've generated a GUID we are preserving it across normalk VM restarts. So I think we should always list it in XML regardless of whether VM is running, because it is telling mgmt app what the VM will be started with next time. Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
On 04/25/2018 04:32 AM, Peter Krempa wrote:
On Tue, Apr 24, 2018 at 15:38:40 -0400, John Ferlan wrote:
On 04/24/2018 03:21 AM, Peter Krempa wrote:
On Mon, Apr 23, 2018 at 20:00:00 -0400, John Ferlan wrote:
The VM Generation ID is a mechanism to provide a unique 128-bit, cryptographically random, and integer value identifier known as the GUID (Globally Unique Identifier) to the guest OS. The value is used to help notify the guest operating system when the virtual machine is executed with a different configuration.
This patch adds support for a new "genid" XML element similar to the "uuid" element. The "genid" element can have two forms "<genid/>" or "<genid>$GUID</genid>". If the $GUID is not provided, libvirt will generate one.
For the ABI checks add avoidance for the genid comparison if the appropriate flag is set.
Since adding support for a generated GUID (or UUID like) value to be displayed only for an active guest, modifying the xml2xml test to include virrandommock.so is necessary since it will generate a "known" UUID value that can be compared against for the active test.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- docs/formatdomain.html.in | 29 ++++++++++++ docs/schemas/domaincommon.rng | 8 ++++ src/conf/domain_conf.c | 59 ++++++++++++++++++++++++ src/conf/domain_conf.h | 3 ++ tests/qemuxml2argvdata/genid-auto.xml | 32 +++++++++++++ tests/qemuxml2argvdata/genid.xml | 32 +++++++++++++ tests/qemuxml2xmloutdata/genid-active.xml | 32 +++++++++++++ tests/qemuxml2xmloutdata/genid-auto-active.xml | 32 +++++++++++++ tests/qemuxml2xmloutdata/genid-auto-inactive.xml | 32 +++++++++++++ tests/qemuxml2xmloutdata/genid-inactive.xml | 32 +++++++++++++ tests/qemuxml2xmltest.c | 5 +- 11 files changed, 295 insertions(+), 1 deletion(-) create mode 100644 tests/qemuxml2argvdata/genid-auto.xml create mode 100644 tests/qemuxml2argvdata/genid.xml create mode 100644 tests/qemuxml2xmloutdata/genid-active.xml create mode 100644 tests/qemuxml2xmloutdata/genid-auto-active.xml create mode 100644 tests/qemuxml2xmloutdata/genid-auto-inactive.xml create mode 100644 tests/qemuxml2xmloutdata/genid-inactive.xml
diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index ada0df227f..6a140f3e40 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -34,6 +34,7 @@ <domain type='kvm' id='1'> <name>MyGuest</name> <uuid>4dea22b3-1d52-d8f3-2516-782e98ab3fa0</uuid> + <genid>43dc0cf8-809b-4adb-9bea-a9abb5f3d90e</genid>
Since we encourage to use the variant with this being empty I'd show that here.
I'd agree except for the fact this is a "live" XML example since "id='1'", so it should stay as is unless of course it's desired to never show the GUID for the running VM.
Hmm, right. It feels slightly wrong though that we are describing configuration on the example of a live XML.
I can remove the id='1' and then use <genid/>... It's not that important to print the GUID to me...
<title>A short description - title - of the domain</title> <description>Some human readable description</description> <metadata>
[...]
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 6d4db50998..8c30adf850 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -18793,6 +18793,34 @@ virDomainDefParseXML(xmlDocPtr xml, VIR_FREE(tmp); }
+ /* Extract domain genid - a genid can either be provided or generated */ + if ((n = virXPathNodeSet("./genid", ctxt, &nodes)) < 0) + goto error; + + if (n > 0) { + if (n != 1) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("element 'genid' can only appear once")); + goto error; + } + def->genidRequested = true; + if (!(tmp = virXPathString("string(./genid[1])", ctxt))) { + if (virUUIDGenerate(def->genid) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + "%s", _("Failed to generate genid"));
Generating this here is pointless, the code using it throws this away and generates a new one. While we don't show this value to the user and thus don't create any false impressions I think that the logic should be shuffled around so that it's generated only when it's required.
How so? Not generating one here would cause active xml-2-xml to fail (as
Basically because a GUID for an inactive definition is nonsense. It will necessarily change upon startup, so having it generated in the parser is pointless.
As noted in the other part of this reply thread - it changes for specific transactions. It's not clear "how" it's initially generated - whether user specified or self generated - only that for certain transitions, it must change and doing so is done by generating a new one.
The parser only needs to restore a user-provided GUID or the one which is store
I expect) because the GUID would be 00000-0000-0000-0000-000000000000. At the end of the series the usage of virUUIDGenerate for ->genid is done for:
1. This code, virDomainDefParseXML 2. virDomainDefCopy when genidRequested and flags & VIR_DOMAIN_DEF_COPY_NEWGENID which is currently only used by the snapshot code, but could be used elsewhere - something I may have been thinking about at least w/r/t one of the qemu_driver paths. 3. qemuProcessGenID *when* flags & VIR_QEMU_PROCESS_START_GEN_VMID
If anything, perhaps what you may be referring to is qemuProcessGenID processing using the VIR_QEMU_PROCESS_START_GEN_VMID flag when called from qemuDomainRevertToSnapshot. The transitions there caused me to be "super cautious", but the Copy, then Start I think upon reflection does overwrite. The same flag is used in qemuDomainSaveImageStartVM, which yes does overwrite, but that's by rule/design.
Perhaps it'll help to summarize the transitions...
Change is not required for the following transitions:
-> Pause/Resume -> VM Reboot
Both above are the same emulator instance.
-> Host reboot
This is wrong. Host reboot causes the VM to be killed. So when booting the GUID will change since we start a new process. While it should not result in any problems if the ID would not change (since the guest OS rebooted anyways) we should treat this as start of the VM.
Well you may call it wrong, but that's what is in the spec.
-> Live migrate
This is technically the same emulator instance. No rollback of execution was possible.
-> Failover in a clustered environment
Thankfully it does not apply at this point since we don't support any of this implicitly. It also probably depends on the way the failover scenario is executed. In the qemu world I read only about coarse-grained lockstepping as a case of failover and that has situations where some rollback could happen, so in that case the VM id should probably change.
But as said, thankfully we don't have to deal with it currenly and also it would not be possible since qemu can't change the id during runtime.
Change is required for the following transitions:
-> Application of offline/online snapshot -> Restoring from backup
This is too vague. If you mean a disk backup, the VM will boot, thus the ID should change (unless the user specified an explicit one).
Hopefully covered in the other half of this thread.
-> Failover to replication target
See above.
-> Importing a VM (or copy or clone)
Change is "Unspecified" when a VM's configuration changes.
So in our case it will change.
It can change based on transitions that require it or it can be used as found in the config file. This is the real bugger in the description.
So the 'design decisions' were:
- For snapshot, the choice was use the virDomainDefCopy and ABI flags.
Actually, the GUID when creating the snapshot is irrelevant. When reverting a snapshot the ID should always change thus it will always require an emulator restart.
Wouldn't the GUID for the creation of the snapshot be whatever was set? The whole snapshot config and domain config saved within is not something I depend on others to understand in greater detail than I do. Still, my choice was alter the GUID when starting and causing an error for the transition to use qemuMonitorLoadSnapshot since we cannot modify the genid. Again, another area that I hoped review would provide details if this was the "wrong choice".
- For the restore from backup, the choice was regenerate and store in the config during startup processing. The docs indicate "The restore sequence will be modified to post process the restore target and apply new generation identifiers to the restored configuration files." - so where it was done would
As pointed out above, this is vague. If it's a restore of the disk state only, the guid will necessarily change in our design when we start the new qemu process.
Cannot disagree - the choice was to change only when/if we restart the VM since that's really the only time it matters.
- It's not 100% clear if we need something special for failover from replication target. Seems like Snapshot or Save/Restore is in the same category, but perhaps there's some hypervisor specific transition.
What is a failover here? You mean if a management starts a new VM after a different host with VM has failed? In such case it's a new start of VM for us and thus will get a new GUID.
It was me commenting and trying not to read too much into things. Perhaps the shorter version of your failover comments from above.
- For import and copy, changing the genid of the copy "seems right". The tricky part is the clone code which would require a separate change to virt-clone since it uses parses and formats XML in separate passes.
That depends on the implementation. Currently yes.
So given all that - are you still of the opinion that the design needs to change even more? If so, then please also characterize your view of how this should work.
Well, that depends. I did not read the docs for this thoroughly enough. If it is okay for us to generate a new GUID upon every boot of a VM then this will be for a rather simple implementation, since we have a very limited set of situations when we are starting a new qemu process which should NOT change the GUID and we will change it in all other scenarios.
It's not really clear that a new GUID for every new boot is "required".
If the documentation does not allow for the above it will be more complex and we'll need to discuss that further.
We shouldn't over complicate things either. John [the rest is covered in the other thread - this is the hazards of cutting threads up like this]
A second consideration then is whether to allow user-specified GUID at all, but I guess mgmt applications may have a different idea when it's necessary to change and thus it makes sense to allow that. For that situation the provided GUID should be always honoured.
This means that we'll probably need a new attribute which will specify that the GUID is custom (or the opposite, that it was generated). If that property is in the default state the startup procedure should generate a new GUID exept for the migration case.
We also might want to consider the managed-save case to bear the same GUID after startup, since we know that we start the new qemu process from the same state as we've saved it.
Add the query of the device objects for the vmgenid device Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_capabilities.c | 2 ++ src/qemu/qemu_capabilities.h | 1 + tests/qemucapabilitiesdata/caps_2.10.0.x86_64.xml | 1 + tests/qemucapabilitiesdata/caps_2.12.0.x86_64.xml | 1 + tests/qemucapabilitiesdata/caps_2.9.0.x86_64.xml | 1 + 5 files changed, 6 insertions(+) diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index 833c75514c..0c1c9cbec5 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -473,6 +473,7 @@ VIR_ENUM_IMPL(virQEMUCaps, QEMU_CAPS_LAST, /* 290 */ "query-cpus-fast", "disk-write-cache", + "vmgenid", ); @@ -1099,6 +1100,7 @@ struct virQEMUCapsStringFlags virQEMUCapsObjectTypes[] = { { "virtio-mouse-ccw", QEMU_CAPS_DEVICE_VIRTIO_MOUSE_CCW }, { "virtio-tablet-ccw", QEMU_CAPS_DEVICE_VIRTIO_TABLET_CCW }, { "pcie-pci-bridge", QEMU_CAPS_DEVICE_PCIE_PCI_BRIDGE }, + { "vmgenid", QEMU_CAPS_DEVICE_VMGENID }, }; static struct virQEMUCapsStringFlags virQEMUCapsDevicePropsVirtioBalloon[] = { diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h index f08cfc2611..a56748dc9b 100644 --- a/src/qemu/qemu_capabilities.h +++ b/src/qemu/qemu_capabilities.h @@ -457,6 +457,7 @@ typedef enum { /* 290 */ QEMU_CAPS_QUERY_CPUS_FAST, /* query-cpus-fast command */ QEMU_CAPS_DISK_WRITE_CACHE, /* qemu block frontends support write-cache param */ + QEMU_CAPS_DEVICE_VMGENID, /* -device vmgenid */ QEMU_CAPS_LAST /* this must always be the last item */ } virQEMUCapsFlags; diff --git a/tests/qemucapabilitiesdata/caps_2.10.0.x86_64.xml b/tests/qemucapabilitiesdata/caps_2.10.0.x86_64.xml index 8dd30ccbcf..acb480d2f0 100644 --- a/tests/qemucapabilitiesdata/caps_2.10.0.x86_64.xml +++ b/tests/qemucapabilitiesdata/caps_2.10.0.x86_64.xml @@ -196,6 +196,7 @@ <flag name='dump-completed'/> <flag name='qcow2-luks'/> <flag name='disk-write-cache'/> + <flag name='vmgenid'/> <version>2010000</version> <kvmVersion>0</kvmVersion> <microcodeVersion>344938</microcodeVersion> diff --git a/tests/qemucapabilitiesdata/caps_2.12.0.x86_64.xml b/tests/qemucapabilitiesdata/caps_2.12.0.x86_64.xml index d809a78380..0f53364679 100644 --- a/tests/qemucapabilitiesdata/caps_2.12.0.x86_64.xml +++ b/tests/qemucapabilitiesdata/caps_2.12.0.x86_64.xml @@ -195,6 +195,7 @@ <flag name='seccomp-blacklist'/> <flag name='query-cpus-fast'/> <flag name='disk-write-cache'/> + <flag name='vmgenid'/> <version>2011090</version> <kvmVersion>0</kvmVersion> <microcodeVersion>390060</microcodeVersion> diff --git a/tests/qemucapabilitiesdata/caps_2.9.0.x86_64.xml b/tests/qemucapabilitiesdata/caps_2.9.0.x86_64.xml index e1dc257a75..8bebd66860 100644 --- a/tests/qemucapabilitiesdata/caps_2.9.0.x86_64.xml +++ b/tests/qemucapabilitiesdata/caps_2.9.0.x86_64.xml @@ -191,6 +191,7 @@ <flag name='isa-serial'/> <flag name='dump-completed'/> <flag name='disk-write-cache'/> + <flag name='vmgenid'/> <version>2009000</version> <kvmVersion>0</kvmVersion> <microcodeVersion>320947</microcodeVersion> -- 2.13.6
Before we generate the command line for qemu, if the domain about to be launched desires to utilize the VM Generation ID functionality, then handle both the regenerating the GUID value for backup recovery (restore operation) and the startup after snapshot as well as checking that the genid value that's about to be placed on the command line doesn't duplicate some other already running domain. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_driver.c | 22 +++++++--- src/qemu/qemu_process.c | 110 +++++++++++++++++++++++++++++++++++++++++++++++- src/qemu/qemu_process.h | 1 + 3 files changed, 126 insertions(+), 7 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index bfb7973100..ee242720dc 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -6591,7 +6591,8 @@ qemuDomainSaveImageStartVM(virConnectPtr conn, if (qemuProcessStart(conn, driver, vm, cookie ? cookie->cpu : NULL, asyncJob, "stdio", *fd, path, NULL, VIR_NETDEV_VPORT_PROFILE_OP_RESTORE, - VIR_QEMU_PROCESS_START_PAUSED) == 0) + VIR_QEMU_PROCESS_START_PAUSED | + VIR_QEMU_PROCESS_START_GEN_VMID) == 0) restored = true; if (intermediatefd != -1) { @@ -15881,6 +15882,7 @@ qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot, * the migratable XML or it will always fail otherwise */ if (config) { bool compatible; + bool abiflags = VIR_DOMAIN_DEF_ABI_CHECK_SKIP_GENID; /* Replace the CPU in config and put the original one in priv * once we're done. When we have the updated CPU def in the @@ -15894,10 +15896,19 @@ qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot, goto endjob; compatible = qemuDomainDefCheckABIStability(driver, vm->def, - config, 0); + config, abiflags); } else { compatible = qemuDomainCheckABIStability(driver, vm, config, - 0); + abiflags); + } + + /* If using VM GenID, there is no way currently to change + * the genid for the running guest, so set an error and + * mark as incompatible. */ + if (compatible && config->genidRequested) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("domain genid update requires restart")); + compatible = false; } if (!compatible) { @@ -15980,7 +15991,8 @@ qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot, cookie ? cookie->cpu : NULL, QEMU_ASYNC_JOB_START, NULL, -1, NULL, snap, VIR_NETDEV_VPORT_PROFILE_OP_CREATE, - VIR_QEMU_PROCESS_START_PAUSED); + VIR_QEMU_PROCESS_START_PAUSED | + VIR_QEMU_PROCESS_START_GEN_VMID); virDomainAuditStart(vm, "from-snapshot", rc >= 0); detail = VIR_DOMAIN_EVENT_STARTED_FROM_SNAPSHOT; event = virDomainEventLifecycleNewFromObj(vm, @@ -16066,7 +16078,7 @@ qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot, VIR_DOMAIN_SNAPSHOT_REVERT_PAUSED)) { /* Flush first event, now do transition 2 or 3 */ bool paused = (flags & VIR_DOMAIN_SNAPSHOT_REVERT_PAUSED) != 0; - unsigned int start_flags = 0; + unsigned int start_flags = VIR_QEMU_PROCESS_START_GEN_VMID; start_flags |= paused ? VIR_QEMU_PROCESS_START_PAUSED : 0; diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 6a5262ae46..06ec4ddeb9 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -5880,6 +5880,107 @@ qemuProcessPrepareHost(virQEMUDriverPtr driver, /** + * qemuProcessCheckGenID: + * @vm: Domain to be checked + * @opaque: Domain about to be started + * + * For each running domain, let's make sure the domain to be started doesn't + * duplicate any running domain's genid GUID value + * + * Returns 0 on success, -1 on failure w/ error message set + */ +static int +qemuProcessCheckGenID(virDomainObjPtr vm, + void *opaque) +{ + int ret = 0; + virDomainObjPtr startvm = opaque; + + /* Ignore ourselves as we're already locked */ + if (vm == startvm) + return 0; + + virObjectLock(vm); + + if (!virDomainObjIsActive(vm)) + goto cleanup; + + if (!vm->def->genidRequested) + goto cleanup; + + if (memcmp(startvm->def->genid, vm->def->genid, VIR_UUID_BUFLEN) == 0) { + /* For a generated value, just change it. Perhaps a result of + * not using virDomainDefCopy which generates a new genid when + * def->genidRequested is true. */ + if (startvm->def->genidGenerated) { + if (virUUIDGenerate(startvm->def->genid) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("failed to regenerate genid")); + ret = -1; + } + } else { + char guidstr[VIR_UUID_STRING_BUFLEN]; + + virUUIDFormat(startvm->def->genid, guidstr); + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("domain '%s' already running with genid '%s'"), + vm->def->name, guidstr); + ret = -1; + } + goto cleanup; + } + + cleanup: + virObjectUnlock(vm); + return ret; +} + + +/** + * qemuProcessGenID: + * @driver: Pointer to driver + * @vm: Pointer to domain object + * @flags: qemuProcessStartFlags + * + * If this domain is requesting to use genid + */ +static int +qemuProcessGenID(virQEMUDriverPtr driver, + virDomainObjPtr vm, + unsigned int flags) +{ + qemuDomainObjPrivatePtr priv = vm->privateData; + + if (!vm->def->genidRequested) + return 0; + + if (!virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DEVICE_VMGENID)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("this QEMU does not support the 'genid' capability")); + return -1; + } + + /* If we are coming from a path where we must provide a new gen id + * value regardless of whether it was previously generated or provided, + * then generate a new GUID value before we build the command line. */ + if (flags & VIR_QEMU_PROCESS_START_GEN_VMID) { + if (virUUIDGenerate(vm->def->genid)) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("failed to regenerate genid")); + return -1; + } + } + + /* Now let's make sure the genid this domain has is not duplicitous + * with something else running. */ + if (virDomainObjListForEach(driver->domains, qemuProcessCheckGenID, vm) < 0) + return -1; + + return 0; +} + + +/** * qemuProcessLaunch: * * Launch a new QEMU process with stopped virtual CPUs. @@ -5931,7 +6032,8 @@ qemuProcessLaunch(virConnectPtr conn, virCheckFlags(VIR_QEMU_PROCESS_START_COLD | VIR_QEMU_PROCESS_START_PAUSED | VIR_QEMU_PROCESS_START_AUTODESTROY | - VIR_QEMU_PROCESS_START_NEW, -1); + VIR_QEMU_PROCESS_START_NEW | + VIR_QEMU_PROCESS_START_GEN_VMID, -1); cfg = virQEMUDriverGetConfig(driver); @@ -5955,6 +6057,9 @@ qemuProcessLaunch(virConnectPtr conn, goto cleanup; logfile = qemuDomainLogContextGetWriteFD(logCtxt); + if (qemuProcessGenID(driver, vm, flags) < 0) + goto cleanup; + VIR_DEBUG("Building emulator command line"); if (!(cmd = qemuBuildCommandLine(driver, qemuDomainLogContextGetManager(logCtxt), @@ -6315,7 +6420,8 @@ qemuProcessStart(virConnectPtr conn, virCheckFlagsGoto(VIR_QEMU_PROCESS_START_COLD | VIR_QEMU_PROCESS_START_PAUSED | - VIR_QEMU_PROCESS_START_AUTODESTROY, cleanup); + VIR_QEMU_PROCESS_START_AUTODESTROY | + VIR_QEMU_PROCESS_START_GEN_VMID, cleanup); if (!migrateFrom && !snapshot) flags |= VIR_QEMU_PROCESS_START_NEW; diff --git a/src/qemu/qemu_process.h b/src/qemu/qemu_process.h index 9dd5c97642..0ebe4cdf2d 100644 --- a/src/qemu/qemu_process.h +++ b/src/qemu/qemu_process.h @@ -79,6 +79,7 @@ typedef enum { VIR_QEMU_PROCESS_START_AUTODESTROY = 1 << 2, VIR_QEMU_PROCESS_START_PRETEND = 1 << 3, VIR_QEMU_PROCESS_START_NEW = 1 << 4, /* internal, new VM is starting */ + VIR_QEMU_PROCESS_START_GEN_VMID = 1 << 5, /* Generate a new VMID */ } qemuProcessStartFlags; int qemuProcessStart(virConnectPtr conn, -- 2.13.6
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_process.c | 68 ++----------------------------------------------- 1 file changed, 2 insertions(+), 66 deletions(-) diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 06ec4ddeb9..bfc22b7d07 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -5880,73 +5880,14 @@ qemuProcessPrepareHost(virQEMUDriverPtr driver, /** - * qemuProcessCheckGenID: - * @vm: Domain to be checked - * @opaque: Domain about to be started - * - * For each running domain, let's make sure the domain to be started doesn't - * duplicate any running domain's genid GUID value - * - * Returns 0 on success, -1 on failure w/ error message set - */ -static int -qemuProcessCheckGenID(virDomainObjPtr vm, - void *opaque) -{ - int ret = 0; - virDomainObjPtr startvm = opaque; - - /* Ignore ourselves as we're already locked */ - if (vm == startvm) - return 0; - - virObjectLock(vm); - - if (!virDomainObjIsActive(vm)) - goto cleanup; - - if (!vm->def->genidRequested) - goto cleanup; - - if (memcmp(startvm->def->genid, vm->def->genid, VIR_UUID_BUFLEN) == 0) { - /* For a generated value, just change it. Perhaps a result of - * not using virDomainDefCopy which generates a new genid when - * def->genidRequested is true. */ - if (startvm->def->genidGenerated) { - if (virUUIDGenerate(startvm->def->genid) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("failed to regenerate genid")); - ret = -1; - } - } else { - char guidstr[VIR_UUID_STRING_BUFLEN]; - - virUUIDFormat(startvm->def->genid, guidstr); - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("domain '%s' already running with genid '%s'"), - vm->def->name, guidstr); - ret = -1; - } - goto cleanup; - } - - cleanup: - virObjectUnlock(vm); - return ret; -} - - -/** * qemuProcessGenID: - * @driver: Pointer to driver * @vm: Pointer to domain object * @flags: qemuProcessStartFlags * * If this domain is requesting to use genid */ static int -qemuProcessGenID(virQEMUDriverPtr driver, - virDomainObjPtr vm, +qemuProcessGenID(virDomainObjPtr vm, unsigned int flags) { qemuDomainObjPrivatePtr priv = vm->privateData; @@ -5971,11 +5912,6 @@ qemuProcessGenID(virQEMUDriverPtr driver, } } - /* Now let's make sure the genid this domain has is not duplicitous - * with something else running. */ - if (virDomainObjListForEach(driver->domains, qemuProcessCheckGenID, vm) < 0) - return -1; - return 0; } @@ -6057,7 +5993,7 @@ qemuProcessLaunch(virConnectPtr conn, goto cleanup; logfile = qemuDomainLogContextGetWriteFD(logCtxt); - if (qemuProcessGenID(driver, vm, flags) < 0) + if (qemuProcessGenID(vm, flags) < 0) goto cleanup; VIR_DEBUG("Building emulator command line"); -- 2.13.6
On Mon, Apr 23, 2018 at 20:00:03 -0400, John Ferlan wrote:
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_process.c | 68 ++----------------------------------------------- 1 file changed, 2 insertions(+), 66 deletions(-)
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 06ec4ddeb9..bfc22b7d07 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -5880,73 +5880,14 @@ qemuProcessPrepareHost(virQEMUDriverPtr driver,
/** - * qemuProcessCheckGenID: - * @vm: Domain to be checked - * @opaque: Domain about to be started
Did you forget to squash this into the previous patch?
On 04/24/2018 02:39 AM, Peter Krempa wrote:
On Mon, Apr 23, 2018 at 20:00:03 -0400, John Ferlan wrote:
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_process.c | 68 ++----------------------------------------------- 1 file changed, 2 insertions(+), 66 deletions(-)
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 06ec4ddeb9..bfc22b7d07 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -5880,73 +5880,14 @@ qemuProcessPrepareHost(virQEMUDriverPtr driver,
/** - * qemuProcessCheckGenID: - * @vm: Domain to be checked - * @opaque: Domain about to be started
Did you forget to squash this into the previous patch?
Yeah - I got totally distracted by all the capabilities adjustments that I forgot to merge this one! John
https://bugzilla.redhat.com/show_bug.cgi?id=1149445 If the domain requests usage of the genid functionality, then add the QEMU '-device vmgenid' to the command line providing either the supplied or generated GUID value. Add tests for both a generated and supplied GUID value. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_command.c | 31 +++++++++++++++++++++++++++++++ tests/qemuxml2argvdata/genid-auto.args | 24 ++++++++++++++++++++++++ tests/qemuxml2argvdata/genid.args | 24 ++++++++++++++++++++++++ tests/qemuxml2argvtest.c | 4 ++++ 4 files changed, 83 insertions(+) create mode 100644 tests/qemuxml2argvdata/genid-auto.args create mode 100644 tests/qemuxml2argvdata/genid.args diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index b666f3715f..1f5e79d86a 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -5942,6 +5942,34 @@ qemuBuildSmbiosCommandLine(virCommandPtr cmd, static int +qemuBuildVMGenIDCommandLine(virCommandPtr cmd, + const virDomainDef *def, + virQEMUCapsPtr qemuCaps) +{ + virBuffer opts = VIR_BUFFER_INITIALIZER; + char guid[VIR_UUID_STRING_BUFLEN]; + + if (!def->genidRequested) + return 0; + + if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_VMGENID)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("genid is not supported with this QEMU binary")); + return -1; + } + + virUUIDFormat(def->genid, guid); + virBufferAsprintf(&opts, "vmgenid,guid=%s,id=vmgenid0", guid); + + virCommandAddArg(cmd, "-device"); + virCommandAddArgBuffer(cmd, &opts); + + virBufferFreeAndReset(&opts); + return 0; +} + + +static int qemuBuildSgaCommandLine(virCommandPtr cmd, const virDomainDef *def, virQEMUCapsPtr qemuCaps) @@ -9869,6 +9897,9 @@ qemuBuildCommandLine(virQEMUDriverPtr driver, if (qemuBuildSmbiosCommandLine(cmd, driver, def) < 0) goto error; + if (qemuBuildVMGenIDCommandLine(cmd, def, qemuCaps) < 0) + goto error; + /* * NB, -nographic *MUST* come before any serial, or monitor * or parallel port flags due to QEMU craziness, where it diff --git a/tests/qemuxml2argvdata/genid-auto.args b/tests/qemuxml2argvdata/genid-auto.args new file mode 100644 index 0000000000..c25b3b87e8 --- /dev/null +++ b/tests/qemuxml2argvdata/genid-auto.args @@ -0,0 +1,24 @@ +LC_ALL=C \ +PATH=/bin \ +HOME=/home/test \ +USER=test \ +LOGNAME=test \ +QEMU_AUDIO_DRV=none \ +/usr/bin/qemu-system-x86_64 \ +-name QEMUGuest1 \ +-S \ +-machine pc,accel=tcg,usb=off,dump-guest-core=off \ +-m 214 \ +-smp 1,sockets=1,cores=1,threads=1 \ +-uuid c7a5fdbd-edaf-9455-926a-d65c16db1809 \ +-device vmgenid,guid=00010203-0405-4607-8809-0a0b0c0d0e0f,id=vmgenid0 \ +-display none \ +-no-user-config \ +-nodefaults \ +-chardev socket,id=charmonitor,path=/tmp/lib/domain--1-QEMUGuest1/monitor.sock,\ +server,nowait \ +-mon chardev=charmonitor,id=monitor,mode=control \ +-rtc base=utc \ +-no-shutdown \ +-boot c \ +-usb diff --git a/tests/qemuxml2argvdata/genid.args b/tests/qemuxml2argvdata/genid.args new file mode 100644 index 0000000000..527e492394 --- /dev/null +++ b/tests/qemuxml2argvdata/genid.args @@ -0,0 +1,24 @@ +LC_ALL=C \ +PATH=/bin \ +HOME=/home/test \ +USER=test \ +LOGNAME=test \ +QEMU_AUDIO_DRV=none \ +/usr/bin/qemu-system-x86_64 \ +-name QEMUGuest1 \ +-S \ +-machine pc,accel=tcg,usb=off,dump-guest-core=off \ +-m 214 \ +-smp 1,sockets=1,cores=1,threads=1 \ +-uuid c7a5fdbd-edaf-9455-926a-d65c16db1809 \ +-device vmgenid,guid=e9392370-2917-565e-692b-d057f46512d6,id=vmgenid0 \ +-display none \ +-no-user-config \ +-nodefaults \ +-chardev socket,id=charmonitor,path=/tmp/lib/domain--1-QEMUGuest1/monitor.sock,\ +server,nowait \ +-mon chardev=charmonitor,id=monitor,mode=control \ +-rtc base=utc \ +-no-shutdown \ +-boot c \ +-usb diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index 74d930ebe2..0dd0850036 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -805,6 +805,10 @@ mymain(void) QEMU_CAPS_SECCOMP_BLACKLIST); DO_TEST_PARSE_ERROR("minimal-no-memory", NONE); DO_TEST("minimal-msg-timestamp", QEMU_CAPS_MSG_TIMESTAMP); + + DO_TEST("genid", QEMU_CAPS_DEVICE_VMGENID); + DO_TEST("genid-auto", QEMU_CAPS_DEVICE_VMGENID); + DO_TEST("machine-aliases1", NONE); DO_TEST("machine-aliases2", QEMU_CAPS_KVM); DO_TEST("machine-core-on", NONE); -- 2.13.6
On Mon, Apr 23, 2018 at 20:00:04 -0400, John Ferlan wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=1149445
If the domain requests usage of the genid functionality, then add the QEMU '-device vmgenid' to the command line providing either the supplied or generated GUID value.
Add tests for both a generated and supplied GUID value.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_command.c | 31 +++++++++++++++++++++++++++++++ tests/qemuxml2argvdata/genid-auto.args | 24 ++++++++++++++++++++++++ tests/qemuxml2argvdata/genid.args | 24 ++++++++++++++++++++++++ tests/qemuxml2argvtest.c | 4 ++++ 4 files changed, 83 insertions(+) create mode 100644 tests/qemuxml2argvdata/genid-auto.args create mode 100644 tests/qemuxml2argvdata/genid.args
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index b666f3715f..1f5e79d86a 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -5942,6 +5942,34 @@ qemuBuildSmbiosCommandLine(virCommandPtr cmd,
static int +qemuBuildVMGenIDCommandLine(virCommandPtr cmd, + const virDomainDef *def, + virQEMUCapsPtr qemuCaps) +{ + virBuffer opts = VIR_BUFFER_INITIALIZER; + char guid[VIR_UUID_STRING_BUFLEN]; + + if (!def->genidRequested) + return 0; + + if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_VMGENID)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("genid is not supported with this QEMU binary")); + return -1;
This is already checked in qemuProcessGenID.
+ } + + virUUIDFormat(def->genid, guid); + virBufferAsprintf(&opts, "vmgenid,guid=%s,id=vmgenid0", guid);
[...]
diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index 74d930ebe2..0dd0850036 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -805,6 +805,10 @@ mymain(void) QEMU_CAPS_SECCOMP_BLACKLIST); DO_TEST_PARSE_ERROR("minimal-no-memory", NONE); DO_TEST("minimal-msg-timestamp", QEMU_CAPS_MSG_TIMESTAMP); + + DO_TEST("genid", QEMU_CAPS_DEVICE_VMGENID); + DO_TEST("genid-auto", QEMU_CAPS_DEVICE_VMGENID);
Please use DO_TEST_CAPS_LATEST/DO_TEST_CAPS_VER
On 04/24/2018 03:28 AM, Peter Krempa wrote:
On Mon, Apr 23, 2018 at 20:00:04 -0400, John Ferlan wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=1149445
If the domain requests usage of the genid functionality, then add the QEMU '-device vmgenid' to the command line providing either the supplied or generated GUID value.
Add tests for both a generated and supplied GUID value.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_command.c | 31 +++++++++++++++++++++++++++++++ tests/qemuxml2argvdata/genid-auto.args | 24 ++++++++++++++++++++++++ tests/qemuxml2argvdata/genid.args | 24 ++++++++++++++++++++++++ tests/qemuxml2argvtest.c | 4 ++++ 4 files changed, 83 insertions(+) create mode 100644 tests/qemuxml2argvdata/genid-auto.args create mode 100644 tests/qemuxml2argvdata/genid.args
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index b666f3715f..1f5e79d86a 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -5942,6 +5942,34 @@ qemuBuildSmbiosCommandLine(virCommandPtr cmd,
static int +qemuBuildVMGenIDCommandLine(virCommandPtr cmd, + const virDomainDef *def, + virQEMUCapsPtr qemuCaps) +{ + virBuffer opts = VIR_BUFFER_INITIALIZER; + char guid[VIR_UUID_STRING_BUFLEN]; + + if (!def->genidRequested) + return 0; + + if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_VMGENID)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("genid is not supported with this QEMU binary")); + return -1;
This is already checked in qemuProcessGenID.
Oh right.
+ } + + virUUIDFormat(def->genid, guid); + virBufferAsprintf(&opts, "vmgenid,guid=%s,id=vmgenid0", guid);
[...]
diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index 74d930ebe2..0dd0850036 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -805,6 +805,10 @@ mymain(void) QEMU_CAPS_SECCOMP_BLACKLIST); DO_TEST_PARSE_ERROR("minimal-no-memory", NONE); DO_TEST("minimal-msg-timestamp", QEMU_CAPS_MSG_TIMESTAMP); + + DO_TEST("genid", QEMU_CAPS_DEVICE_VMGENID); + DO_TEST("genid-auto", QEMU_CAPS_DEVICE_VMGENID);
Please use DO_TEST_CAPS_LATEST/DO_TEST_CAPS_VER
OK - That was added after I originally posted. Happy, happy, joy, joy, genid is the guinea pig! Still, it's not clear what would be expected since the comments indicate desired usage for positive test cases. Since pre 2.9 would be a failure scenario, the only difference is works or doesn't work, which honestly doesn't appear to be the intended purpose of the new test macros. So do you expect to see _VER for all versions since 2.9 as well or just a DO_TEST_CAPS_LATEST("genid"); (and "genid-auto") with adjusted result file names instead of the DO_TEST results? Wish there were more examples other than just disk-drive-write-cache... The 2.9 output is different from 2.10 and 2.12, but does that matter for this code? We don't have a comparable 2.11 output. Seems like an excessive amount of *.args files could be generated with the only difference being the -machine output. Although I do note that the *disk-drive-write-cache*.*.args files all have the same machine id value while my branch has different ones for each version file. That's because you define the machine in the XML file while mine just used 'pc'. Leaves me wondering about the validity of what was being done /-|. Thinking forward to when/if the next set of qemu capabilities are created, does that mean we have to go back and create 2.12 specific files for the current "x86_64-latest.args" file(s) since the latest will invariably and eventually get some new stuff that will cause a difference? Who gets that task? The first unlucky/unfortunate sole that creates the caps_2.13.0.x86_64.xml file? Or whomever has a change causing the difference? To a degree it's fortunate that genid is only in the x86_64 capabilities I guess; otherwise, I'd be wondering again asking/pondering about aarch64, s390x, ppc64 too... John
On Tue, Apr 24, 2018 at 16:29:45 -0400, John Ferlan wrote:
On 04/24/2018 03:28 AM, Peter Krempa wrote:
On Mon, Apr 23, 2018 at 20:00:04 -0400, John Ferlan wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=1149445
If the domain requests usage of the genid functionality, then add the QEMU '-device vmgenid' to the command line providing either the supplied or generated GUID value.
Add tests for both a generated and supplied GUID value.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_command.c | 31 +++++++++++++++++++++++++++++++ tests/qemuxml2argvdata/genid-auto.args | 24 ++++++++++++++++++++++++ tests/qemuxml2argvdata/genid.args | 24 ++++++++++++++++++++++++ tests/qemuxml2argvtest.c | 4 ++++ 4 files changed, 83 insertions(+) create mode 100644 tests/qemuxml2argvdata/genid-auto.args create mode 100644 tests/qemuxml2argvdata/genid.args
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index b666f3715f..1f5e79d86a 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -5942,6 +5942,34 @@ qemuBuildSmbiosCommandLine(virCommandPtr cmd,
static int +qemuBuildVMGenIDCommandLine(virCommandPtr cmd, + const virDomainDef *def, + virQEMUCapsPtr qemuCaps) +{ + virBuffer opts = VIR_BUFFER_INITIALIZER; + char guid[VIR_UUID_STRING_BUFLEN]; + + if (!def->genidRequested) + return 0; + + if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_VMGENID)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("genid is not supported with this QEMU binary")); + return -1;
This is already checked in qemuProcessGenID.
Oh right.
+ } + + virUUIDFormat(def->genid, guid); + virBufferAsprintf(&opts, "vmgenid,guid=%s,id=vmgenid0", guid);
[...]
diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index 74d930ebe2..0dd0850036 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -805,6 +805,10 @@ mymain(void) QEMU_CAPS_SECCOMP_BLACKLIST); DO_TEST_PARSE_ERROR("minimal-no-memory", NONE); DO_TEST("minimal-msg-timestamp", QEMU_CAPS_MSG_TIMESTAMP); + + DO_TEST("genid", QEMU_CAPS_DEVICE_VMGENID); + DO_TEST("genid-auto", QEMU_CAPS_DEVICE_VMGENID);
Please use DO_TEST_CAPS_LATEST/DO_TEST_CAPS_VER
OK - That was added after I originally posted. Happy, happy, joy, joy, genid is the guinea pig! Still, it's not clear what would be expected since the comments indicate desired usage for positive test cases.
Since pre 2.9 would be a failure scenario, the only difference is works or doesn't work, which honestly doesn't appear to be the intended purpose of the new test macros.
No, the current set of macros does not have the negative option. Given that you get a error from a capability check I don't think it makes much sense to test that scenario.
So do you expect to see _VER for all versions since 2.9 as well or just a DO_TEST_CAPS_LATEST("genid"); (and "genid-auto") with adjusted result file names instead of the DO_TEST results? Wish there were more examples other than just disk-drive-write-cache...
I honestly think that a DO_TEST_CAPS_LATEST check is enough in this case.
The 2.9 output is different from 2.10 and 2.12, but does that matter for this code? We don't have a comparable 2.11 output. Seems like an
No the output is different because of other factors. That means that a LATEST check is okay.
excessive amount of *.args files could be generated with the only difference being the -machine output. Although I do note that the
If your output differs on the -machine bit, it will in every other release. You need to specify an explicit machine type rather than the alias.
*disk-drive-write-cache*.*.args files all have the same machine id value while my branch has different ones for each version file. That's because you define the machine in the XML file while mine just used 'pc'. Leaves me wondering about the validity of what was being done /-|.
Erm .... just use an explicit machine type and not the alias.
Thinking forward to when/if the next set of qemu capabilities are created, does that mean we have to go back and create 2.12 specific files for the current "x86_64-latest.args" file(s) since the latest will invariably and eventually get some new stuff that will cause a difference? Who gets that task? The first unlucky/unfortunate sole that creates the caps_2.13.0.x86_64.xml file? Or whomever has a change causing the difference?
All this was discussed in the thread adding the testing infrastructure. The new files should be created/forked prior to adding new code which will change the output. The task is on the author of the patches that will modify the code in such way that changes the output. Note that just adding a new capability file should NOT change this testing except for cases when the new capabilities would drop a previously existing capability bit. In such case it is actually worth reviewing.
To a degree it's fortunate that genid is only in the x86_64 capabilities I guess; otherwise, I'd be wondering again asking/pondering about aarch64, s390x, ppc64 too...
That depends on how much do you care about those. The CAPS testing infrastructure supports other arches too, I just did not create the simpler macros for it yet.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- docs/news.xml | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/docs/news.xml b/docs/news.xml index dec3f134ce..c76844ef06 100644 --- a/docs/news.xml +++ b/docs/news.xml @@ -56,6 +56,18 @@ host-passthrough CPU model. </description> </change> + <change> + <summary> + Add support for VM Generation ID + </summary> + <description> + The VM Generatation ID exposes a 128-bit, cryptographically + random, integer value identifier, referred to as a Globally + Unique Identifier (GUID) to the guest in order to notify the + guest operating system when the virtual machine is executed + with a different configuration. + </description> + </change> </section> <section title="Removed features"> <change> -- 2.13.6
participants (3)
-
Daniel P. Berrangé -
John Ferlan -
Peter Krempa