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 56ac64630f..c417680dac 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -6611,7 +6611,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) {
@@ -15873,6 +15874,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
@@ -15886,10 +15888,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) {
@@ -15972,7 +15983,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,
@@ -16058,7 +16070,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 f02114c693..20fcdf7f26 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -5882,6 +5882,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.
@@ -5933,7 +6034,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);
@@ -5957,6 +6059,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),
@@ -6317,7 +6422,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 2741115673..30a73ff3f1 100644
--- a/src/qemu/qemu_process.h
+++ b/src/qemu/qemu_process.h
@@ -78,6 +78,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
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On Wed, Apr 11, 2018 at 04:10:03PM -0400, John Ferlan wrote: > 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(-) > + /* 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; Is this really required ? AFAIK, the gen ID merely has to be unique within the scope of the installed guest disk image, not amongst all VMs that exist. If it was the latter, the check we're doing is pretty weak because it only considers one host, not VMs on every other host. IMHO this check is not desirable because it adds mutex contention against every guest, into the start up path and other code paths too. 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 :| -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On 04/12/2018 07:13 AM, Daniel P. Berrangé wrote: > On Wed, Apr 11, 2018 at 04:10:03PM -0400, John Ferlan wrote: >> 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(-) > > >> + /* 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; > > Is this really required ? AFAIK, the gen ID merely has to be unique within > the scope of the installed guest disk image, not amongst all VMs that exist. > If it was the latter, the check we're doing is pretty weak because it only > considers one host, not VMs on every other host. IMHO this check is not > desirable because it adds mutex contention against every guest, into the > start up path and other code paths too. > Well it wasn't 100% clear from what I read whether it would be required. I may have misread something along the way too with how/why the value was being used with Active Directory and perhaps a cloned domain using (or needing) some mechanism to distinguish between the two. Still, in rereading the MS GenID paper again uniqueness between two domains doesn't seem to be required (which is fine by me - I didn't like the code either) since the AD issues seem to be time based to rolling back using a snapshot. John FWIW: http://go.microsoft.com/fwlink/?LinkId=260709 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On Thu, Apr 12, 2018 at 07:54:57AM -0400, John Ferlan wrote: > > > On 04/12/2018 07:13 AM, Daniel P. Berrangé wrote: > > On Wed, Apr 11, 2018 at 04:10:03PM -0400, John Ferlan wrote: > >> 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(-) > > > > > >> + /* 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; > > > > Is this really required ? AFAIK, the gen ID merely has to be unique within > > the scope of the installed guest disk image, not amongst all VMs that exist. > > If it was the latter, the check we're doing is pretty weak because it only > > considers one host, not VMs on every other host. IMHO this check is not > > desirable because it adds mutex contention against every guest, into the > > start up path and other code paths too. > > > > Well it wasn't 100% clear from what I read whether it would be required. > I may have misread something along the way too with how/why the value > was being used with Active Directory and perhaps a cloned domain using > (or needing) some mechanism to distinguish between the two. > > Still, in rereading the MS GenID paper again uniqueness between two > domains doesn't seem to be required (which is fine by me - I didn't like > the code either) since the AD issues seem to be time based to rolling > back using a snapshot. In practice, unless someone uses tragically bad UUID generator that produces clashing strings, they're all going to be unique anyway. So I think we're safe to drop the check and blame any mistakes on the mgmt app using libvirt, if they occurr. 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 :| -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
© 2016 - 2025 Red Hat, Inc.