[libvirt] [PATCH v2 0/7] qemu: Add support for -incoming defer
Traditionally, we pass incoming migration URI on QEMU command line, which has some drawbacks. Depending on the URI QEMU may initialize its migration state immediately without giving us a chance to set any additional migration parameters (this applies mainly for fd: URIs). For some URIs the monitor may be completely blocked from the beginning until migration is finished, which means we may be stuck in qmp_capabilities command without being able to send any QMP commands. QEMU solved this by introducing "defer" parameter for -incoming command line option. This will tell QEMU to prepare for an incoming migration while the actual incoming URI is sent using migrate-incoming QMP command. Before calling this command we can normally talk to the monitor and even set any migration parameters which will be honored by the incoming migration. The first 17 patches from the original series were already pushed. This is the rest of the series. See individual patches for changes from previous version. Jiri Denemark (7): qemu: Introduce qemuProcessInit qemu: Introduce qemuProcessLaunch qemu: Introduce qemuProcessFinishStartup qemu: Separate incoming URI generation from qemuMigrationPrepareAny qemu: Kill QEMU process if Prepare phase fails qemu: Skip starting NBD servers for offline migration qemu: Use qemuProcessLaunch in migration Prepare phase src/qemu/qemu_migration.c | 250 ++++++++++++++---------- src/qemu/qemu_process.c | 488 +++++++++++++++++++++++++++++----------------- src/qemu/qemu_process.h | 19 ++ 3 files changed, 467 insertions(+), 290 deletions(-) -- 2.6.3
qemuProcessStart is going to be split in three parts: qemuProcessInit, qemuProcessLaunch, and qemuProcessFinish so that migration Prepare phase can insert additional code in the process. qemuProcessStart will be a small wrapper for all other callers. qemuProcessInit prepares the domain up to the point when priv->qemuCaps is initialized. Signed-off-by: Jiri Denemark <jdenemar@redhat.com> --- Notes: Version 2: - avoid calling qemuProcessStop when starting fails with "VM is already active" src/qemu/qemu_process.c | 133 ++++++++++++++++++++++++++++++++---------------- src/qemu/qemu_process.h | 4 ++ 2 files changed, 92 insertions(+), 45 deletions(-) diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 2192ad8..784ae08 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -4476,6 +4476,86 @@ qemuProcessMakeDir(virQEMUDriverPtr driver, } +/** + * qemuProcessInit: + * + * Prepares the domain up to the point when priv->qemuCaps is initialized. + * + * @returns 0 on success, + * -1 on error requiring qemuProcessStop to be called, + * -2 on error when qemuProcessStop must NOT be called. + */ +int +qemuProcessInit(virQEMUDriverPtr driver, + virDomainObjPtr vm, + bool migration) +{ + virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver); + virCapsPtr caps = NULL; + qemuDomainObjPrivatePtr priv = vm->privateData; + int ret = -2; + + VIR_DEBUG("vm=%p name=%s id=%d migration=%d", + vm, vm->def->name, vm->def->id, migration); + + VIR_DEBUG("Beginning VM startup process"); + + if (virDomainObjIsActive(vm)) { + virReportError(VIR_ERR_OPERATION_INVALID, "%s", + _("VM is already active")); + goto cleanup; + } + + /* require qemuProcessStop to be called from now on */ + ret = -1; + + if (!(caps = virQEMUDriverGetCapabilities(driver, false))) + goto cleanup; + + /* Some things, paths, ... are generated here and we want them to persist. + * Fill them in prior to setting the domain def as transient. */ + VIR_DEBUG("Generating paths"); + + if (qemuPrepareNVRAM(cfg, vm, migration) < 0) + goto cleanup; + + /* Do this upfront, so any part of the startup process can add + * runtime state to vm->def that won't be persisted. This let's us + * report implicit runtime defaults in the XML, like vnc listen/socket + */ + VIR_DEBUG("Setting current domain def as transient"); + if (virDomainObjSetDefTransient(caps, driver->xmlopt, vm, true) < 0) + goto cleanup; + + vm->def->id = qemuDriverAllocateID(driver); + qemuDomainSetFakeReboot(driver, vm, false); + virDomainObjSetState(vm, VIR_DOMAIN_PAUSED, VIR_DOMAIN_PAUSED_STARTING_UP); + + if (virAtomicIntInc(&driver->nactive) == 1 && driver->inhibitCallback) + driver->inhibitCallback(true, driver->inhibitOpaque); + + /* Run an early hook to set-up missing devices */ + if (qemuProcessStartHook(driver, vm, + VIR_HOOK_QEMU_OP_PREPARE, + VIR_HOOK_SUBOP_BEGIN) < 0) + goto cleanup; + + VIR_DEBUG("Determining emulator version"); + virObjectUnref(priv->qemuCaps); + if (!(priv->qemuCaps = virQEMUCapsCacheLookupCopy(driver->qemuCapsCache, + vm->def->emulator, + vm->def->os.machine))) + goto cleanup; + + ret = 0; + + cleanup: + virObjectUnref(cfg); + virObjectUnref(caps); + return ret; +} + + int qemuProcessStart(virConnectPtr conn, virQEMUDriverPtr driver, virDomainObjPtr vm, @@ -4498,7 +4578,7 @@ int qemuProcessStart(virConnectPtr conn, size_t i; char *nodeset = NULL; unsigned int stop_flags; - virQEMUDriverConfigPtr cfg; + virQEMUDriverConfigPtr cfg = NULL; virCapsPtr caps = NULL; unsigned int hostdev_flags = 0; size_t nnicindexes = 0; @@ -4516,6 +4596,13 @@ int qemuProcessStart(virConnectPtr conn, VIR_QEMU_PROCESS_START_PAUSED | VIR_QEMU_PROCESS_START_AUTODESTROY, -1); + if ((rv = qemuProcessInit(driver, vm, !!migrateFrom)) < 0) { + if (rv == -2) + goto cleanup; + else + goto error; + } + cfg = virQEMUDriverGetConfig(driver); /* From now on until domain security labeling is done: @@ -4534,53 +4621,9 @@ int qemuProcessStart(virConnectPtr conn, /* We don't increase cfg's reference counter here. */ hookData.cfg = cfg; - VIR_DEBUG("Beginning VM startup process"); - - if (virDomainObjIsActive(vm)) { - virReportError(VIR_ERR_OPERATION_INVALID, - "%s", _("VM is already active")); - virObjectUnref(cfg); - return -1; - } - if (!(caps = virQEMUDriverGetCapabilities(driver, false))) goto error; - /* Some things, paths, ... are generated here and we want them to persist. - * Fill them in prior to setting the domain def as transient. */ - VIR_DEBUG("Generating paths"); - - if (qemuPrepareNVRAM(cfg, vm, !!migrateFrom) < 0) - goto error; - - /* Do this upfront, so any part of the startup process can add - * runtime state to vm->def that won't be persisted. This let's us - * report implicit runtime defaults in the XML, like vnc listen/socket - */ - VIR_DEBUG("Setting current domain def as transient"); - if (virDomainObjSetDefTransient(caps, driver->xmlopt, vm, true) < 0) - goto error; - - vm->def->id = qemuDriverAllocateID(driver); - qemuDomainSetFakeReboot(driver, vm, false); - virDomainObjSetState(vm, VIR_DOMAIN_PAUSED, VIR_DOMAIN_PAUSED_STARTING_UP); - - if (virAtomicIntInc(&driver->nactive) == 1 && driver->inhibitCallback) - driver->inhibitCallback(true, driver->inhibitOpaque); - - /* Run an early hook to set-up missing devices */ - if (qemuProcessStartHook(driver, vm, - VIR_HOOK_QEMU_OP_PREPARE, - VIR_HOOK_SUBOP_BEGIN) < 0) - goto error; - - VIR_DEBUG("Determining emulator version"); - virObjectUnref(priv->qemuCaps); - if (!(priv->qemuCaps = virQEMUCapsCacheLookupCopy(driver->qemuCapsCache, - vm->def->emulator, - vm->def->os.machine))) - goto error; - /* network devices must be "prepared" before hostdevs, because * setting up a network device might create a new hostdev that * will need to be setup. diff --git a/src/qemu/qemu_process.h b/src/qemu/qemu_process.h index dcb7e28..3948caf 100644 --- a/src/qemu/qemu_process.h +++ b/src/qemu/qemu_process.h @@ -81,6 +81,10 @@ int qemuProcessStart(virConnectPtr conn, virNetDevVPortProfileOp vmop, unsigned int flags); +int qemuProcessInit(virQEMUDriverPtr driver, + virDomainObjPtr vm, + bool migration); + typedef enum { VIR_QEMU_PROCESS_STOP_MIGRATED = 1 << 0, VIR_QEMU_PROCESS_STOP_NO_RELABEL = 1 << 1, -- 2.6.3
On Thu, Nov 19, 2015 at 01:09:15PM +0100, Jiri Denemark wrote:
qemuProcessStart is going to be split in three parts: qemuProcessInit, qemuProcessLaunch, and qemuProcessFinish so that migration Prepare phase can insert additional code in the process. qemuProcessStart will be a small wrapper for all other callers.
qemuProcessInit prepares the domain up to the point when priv->qemuCaps is initialized.
Signed-off-by: Jiri Denemark <jdenemar@redhat.com> ---
Notes: Version 2: - avoid calling qemuProcessStop when starting fails with "VM is already active"
src/qemu/qemu_process.c | 133 ++++++++++++++++++++++++++++++++---------------- src/qemu/qemu_process.h | 4 ++ 2 files changed, 92 insertions(+), 45 deletions(-)
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 2192ad8..784ae08 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -4476,6 +4476,86 @@ qemuProcessMakeDir(virQEMUDriverPtr driver, }
+/** + * qemuProcessInit: + * + * Prepares the domain up to the point when priv->qemuCaps is initialized. + * + * @returns 0 on success, + * -1 on error requiring qemuProcessStop to be called, + * -2 on error when qemuProcessStop must NOT be called. + */
I don't like this as there is no benefit from letting the caller to decide whether qemuProcessStop needs to be called or not. Why don't just do it inside qemuProcessInit and add a comment that qemuProcessStop must not be called at all if this function fails? I know, that last patch updates qemuMigrationPrepareAny and that patch 5/7 joined 'stop' with 'endjob' but that's not necessary too. Just jump to 'stop' instead to 'stopjob'. Then it would be possible in case of qemuProcessInit fails jump to 'endjob' in qemuMigrationPrepareAny. Pavel [...]
Once qemuProcessInit was called, qemuProcessLaunch will launch a new QEMU process with stopped virtual CPUs. Signed-off-by: Jiri Denemark <jdenemar@redhat.com> --- Notes: Version 2: - save status before running VIR_HOOK_QEMU_OP_STARTED hook - call virSecurityManagerSetAllLabel even if !incoming - move qemuProcessStop out of qemuProcessLaunch to avoid it being called twice src/qemu/qemu_process.c | 323 ++++++++++++++++++++++++++++-------------------- src/qemu/qemu_process.h | 9 ++ 2 files changed, 199 insertions(+), 133 deletions(-) diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 784ae08..3066692 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -4556,16 +4556,28 @@ qemuProcessInit(virQEMUDriverPtr driver, } -int qemuProcessStart(virConnectPtr conn, - virQEMUDriverPtr driver, - virDomainObjPtr vm, - qemuDomainAsyncJob asyncJob, - const char *migrateFrom, - int migrateFd, - const char *migratePath, - virDomainSnapshotObjPtr snapshot, - virNetDevVPortProfileOp vmop, - unsigned int flags) +/** + * qemuProcessLaunch: + * + * Launch a new QEMU process with stopped virtual CPUs. + * + * The caller is supposed to call qemuProcessStop with appropriate + * flags in case of failure. + * + * @returns 0 on success, + * -1 on error which happened before devices were labeled and thus + * there is no need to restore them, + * -2 on error requesting security labels to be restored. + */ +int +qemuProcessLaunch(virConnectPtr conn, + virQEMUDriverPtr driver, + virDomainObjPtr vm, + qemuDomainAsyncJob asyncJob, + qemuProcessIncomingDefPtr incoming, + virDomainSnapshotObjPtr snapshot, + virNetDevVPortProfileOp vmop, + unsigned int flags) { int ret = -1; int rv; @@ -4577,18 +4589,22 @@ int qemuProcessStart(virConnectPtr conn, struct qemuProcessHookData hookData; size_t i; char *nodeset = NULL; - unsigned int stop_flags; - virQEMUDriverConfigPtr cfg = NULL; + virQEMUDriverConfigPtr cfg; virCapsPtr caps = NULL; unsigned int hostdev_flags = 0; size_t nnicindexes = 0; int *nicindexes = NULL; - qemuProcessIncomingDefPtr incoming = NULL; - VIR_DEBUG("vm=%p name=%s id=%d asyncJob=%d migrateFrom=%s migrateFd=%d " - "migratePath=%s snapshot=%p vmop=%d flags=0x%x", - vm, vm->def->name, vm->def->id, asyncJob, NULLSTR(migrateFrom), - migrateFd, NULLSTR(migratePath), snapshot, vmop, flags); + VIR_DEBUG("vm=%p name=%s id=%d asyncJob=%d " + "incoming.launchURI=%s incoming.deferredURI=%s " + "incoming.fd=%d incoming.path=%s " + "snapshot=%p vmop=%d flags=0x%x", + vm, vm->def->name, vm->def->id, asyncJob, + NULLSTR(incoming ? incoming->launchURI : NULL), + NULLSTR(incoming ? incoming->deferredURI : NULL), + incoming ? incoming->fd : -1, + NULLSTR(incoming ? incoming->path : NULL), + snapshot, vmop, flags); /* Okay, these are just internal flags, * but doesn't hurt to check */ @@ -4596,25 +4612,8 @@ int qemuProcessStart(virConnectPtr conn, VIR_QEMU_PROCESS_START_PAUSED | VIR_QEMU_PROCESS_START_AUTODESTROY, -1); - if ((rv = qemuProcessInit(driver, vm, !!migrateFrom)) < 0) { - if (rv == -2) - goto cleanup; - else - goto error; - } - cfg = virQEMUDriverGetConfig(driver); - /* From now on until domain security labeling is done: - * if any operation fails and we goto cleanup, we must not - * restore any security label as we would overwrite labels - * we did not set. */ - stop_flags = VIR_QEMU_PROCESS_STOP_NO_RELABEL; - /* If we fail while doing incoming migration, then we must not - * relabel, as the source is still using the files. */ - if (migrateFrom) - stop_flags |= VIR_QEMU_PROCESS_STOP_MIGRATED; - hookData.conn = conn; hookData.vm = vm; hookData.driver = driver; @@ -4622,7 +4621,7 @@ int qemuProcessStart(virConnectPtr conn, hookData.cfg = cfg; if (!(caps = virQEMUDriverGetCapabilities(driver, false))) - goto error; + goto cleanup; /* network devices must be "prepared" before hostdevs, because * setting up a network device might create a new hostdev that @@ -4630,35 +4629,35 @@ int qemuProcessStart(virConnectPtr conn, */ VIR_DEBUG("Preparing network devices"); if (qemuNetworkPrepareDevices(vm->def) < 0) - goto error; + goto cleanup; /* Must be run before security labelling */ VIR_DEBUG("Preparing host devices"); if (!cfg->relaxedACS) hostdev_flags |= VIR_HOSTDEV_STRICT_ACS_CHECK; - if (!migrateFrom) + if (!incoming) hostdev_flags |= VIR_HOSTDEV_COLD_BOOT; if (qemuHostdevPrepareDomainDevices(driver, vm->def, priv->qemuCaps, hostdev_flags) < 0) - goto error; + goto cleanup; VIR_DEBUG("Preparing chr devices"); if (virDomainChrDefForeach(vm->def, true, qemuProcessPrepareChardevDevice, NULL) < 0) - goto error; + goto cleanup; VIR_DEBUG("Checking domain and device security labels"); if (virSecurityManagerCheckAllLabel(driver->securityManager, vm->def) < 0) - goto error; + goto cleanup; /* If you are using a SecurityDriver with dynamic labelling, then generate a security label for isolation */ VIR_DEBUG("Generating domain security label (if required)"); if (virSecurityManagerGenLabel(driver->securityManager, vm->def) < 0) { virDomainAuditSecurityLabel(vm, false); - goto error; + goto cleanup; } virDomainAuditSecurityLabel(vm, true); @@ -4667,14 +4666,14 @@ int qemuProcessStart(virConnectPtr conn, char *hugepagePath = qemuGetHugepagePath(&cfg->hugetlbfs[i]); if (!hugepagePath) - goto error; + goto cleanup; if (virSecurityManagerSetHugepages(driver->securityManager, vm->def, hugepagePath) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Unable to set huge path in security driver")); VIR_FREE(hugepagePath); - goto error; + goto cleanup; } VIR_FREE(hugepagePath); } @@ -4687,18 +4686,18 @@ int qemuProcessStart(virConnectPtr conn, VIR_DEBUG("Setting up ports for graphics"); if (qemuProcessSetupGraphics(driver, vm) < 0) - goto error; + goto cleanup; if (virFileMakePath(cfg->logDir) < 0) { virReportSystemError(errno, _("cannot create log directory %s"), cfg->logDir); - goto error; + goto cleanup; } VIR_DEBUG("Creating domain log file"); if ((logfile = qemuDomainCreateLog(driver, vm, false)) < 0) - goto error; + goto cleanup; if (vm->def->virtType == VIR_DOMAIN_VIRT_KVM) { VIR_DEBUG("Checking for KVM availability"); @@ -4707,15 +4706,15 @@ int qemuProcessStart(virConnectPtr conn, _("Domain requires KVM, but it is not available. " "Check that virtualization is enabled in the host BIOS, " "and host configuration is setup to load the kvm modules.")); - goto error; + goto cleanup; } } if (!qemuValidateCpuMax(vm->def, priv->qemuCaps)) - goto error; + goto cleanup; if (qemuAssignDeviceAliases(vm->def, priv->qemuCaps) < 0) - goto error; + goto cleanup; /* Get the advisory nodeset from numad if 'placement' of * either <vcpu> or <numatune> is 'auto'. @@ -4724,48 +4723,48 @@ int qemuProcessStart(virConnectPtr conn, nodeset = virNumaGetAutoPlacementAdvice(vm->def->vcpus, virDomainDefGetMemoryActual(vm->def)); if (!nodeset) - goto error; + goto cleanup; VIR_DEBUG("Nodeset returned from numad: %s", nodeset); if (virBitmapParse(nodeset, 0, &priv->autoNodeset, VIR_DOMAIN_CPUMASK_LEN) < 0) - goto error; + goto cleanup; if (!(priv->autoCpuset = virCapabilitiesGetCpusForNodemask(caps, priv->autoNodeset))) - goto error; + goto cleanup; } - if (!migrateFrom && !snapshot && + if (!incoming && !snapshot && virDomainDefCheckDuplicateDiskInfo(vm->def) < 0) - goto error; + goto cleanup; /* "volume" type disk's source must be translated before * cgroup and security setting. */ for (i = 0; i < vm->def->ndisks; i++) { if (virStorageTranslateDiskSourcePool(conn, vm->def->disks[i]) < 0) - goto error; + goto cleanup; } if (qemuDomainCheckDiskPresence(driver, vm, flags & VIR_QEMU_PROCESS_START_COLD) < 0) - goto error; + goto cleanup; if (vm->def->mem.min_guarantee) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("Parameter 'min_guarantee' " "not supported by QEMU.")); - goto error; + goto cleanup; } if (VIR_ALLOC(priv->monConfig) < 0) - goto error; + goto cleanup; VIR_DEBUG("Preparing monitor state"); if (qemuProcessPrepareMonitorChr(cfg, priv->monConfig, vm->def->name) < 0) - goto error; + goto cleanup; priv->monJSON = virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_MONITOR_JSON); priv->monError = false; @@ -4776,7 +4775,7 @@ int qemuProcessStart(virConnectPtr conn, if (!(priv->pidfile = virPidFileBuildPath(cfg->stateDir, vm->def->name))) { virReportSystemError(errno, "%s", _("Failed to build pidfile path.")); - goto error; + goto cleanup; } if (unlink(priv->pidfile) < 0 && @@ -4784,7 +4783,7 @@ int qemuProcessStart(virConnectPtr conn, virReportSystemError(errno, _("Cannot remove stale PID file %s"), priv->pidfile); - goto error; + goto cleanup; } /* @@ -4797,14 +4796,7 @@ int qemuProcessStart(virConnectPtr conn, if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DEVICE)) { VIR_DEBUG("Assigning domain PCI addresses"); if ((qemuDomainAssignAddresses(vm->def, priv->qemuCaps, vm)) < 0) - goto error; - } - - if (migrateFrom) { - incoming = qemuProcessIncomingDefNew(priv->qemuCaps, migrateFrom, - migrateFd, migratePath); - if (!incoming) - goto error; + goto cleanup; } VIR_DEBUG("Building emulator command line"); @@ -4816,7 +4808,7 @@ int qemuProcessStart(virConnectPtr conn, qemuCheckFips(), priv->autoNodeset, &nnicindexes, &nicindexes))) - goto error; + goto cleanup; if (incoming && incoming->fd != -1) virCommandPassFD(cmd, incoming->fd, 0); @@ -4827,13 +4819,13 @@ int qemuProcessStart(virConnectPtr conn, */ if (qemuProcessMakeDir(driver, vm, cfg->libDir) < 0 || qemuProcessMakeDir(driver, vm, cfg->channelTargetDir) < 0) - goto error; + goto cleanup; /* now that we know it is about to start call the hook if present */ if (qemuProcessStartHook(driver, vm, VIR_HOOK_QEMU_OP_START, VIR_HOOK_SUBOP_BEGIN) < 0) - goto error; + goto cleanup; qemuLogOperation(vm, "starting up", logfile, cmd); @@ -4850,7 +4842,7 @@ int qemuProcessStart(virConnectPtr conn, VIR_DEBUG("Setting up raw IO"); if (qemuProcessSetupRawIO(driver, vm, cmd) < 0) - goto error; + goto cleanup; virCommandSetPreExecHook(cmd, qemuProcessHook, &hookData); virCommandSetMaxProcesses(cmd, cfg->maxProcesses); @@ -4860,7 +4852,7 @@ int qemuProcessStart(virConnectPtr conn, VIR_DEBUG("Setting up security labelling"); if (virSecurityManagerSetChildProcessLabel(driver->securityManager, vm->def, cmd) < 0) - goto error; + goto cleanup; virCommandSetOutputFD(cmd, &logfile); virCommandSetErrorFD(cmd, &logfile); @@ -4870,7 +4862,7 @@ int qemuProcessStart(virConnectPtr conn, virCommandRequireHandshake(cmd); if (virSecurityManagerPreFork(driver->securityManager) < 0) - goto error; + goto cleanup; rv = virCommandRun(cmd, NULL); virSecurityManagerPostFork(driver->securityManager); @@ -4890,37 +4882,38 @@ int qemuProcessStart(virConnectPtr conn, VIR_DEBUG("Writing early domain status to disk"); if (virDomainSaveStatus(driver->xmlopt, cfg->stateDir, vm) < 0) - goto error; + goto cleanup; VIR_DEBUG("Waiting for handshake from child"); if (virCommandHandshakeWait(cmd) < 0) { /* Read errors from child that occurred between fork and exec. */ qemuProcessReadChildErrors(driver, vm, pos); - goto error; + goto cleanup; } VIR_DEBUG("Setting up domain cgroup (if required)"); if (qemuSetupCgroup(driver, vm, nnicindexes, nicindexes) < 0) - goto error; + goto cleanup; /* This must be done after cgroup placement to avoid resetting CPU * affinity */ if (!vm->def->cputune.emulatorpin && qemuProcessInitCpuAffinity(vm) < 0) - goto error; + goto cleanup; VIR_DEBUG("Setting domain security labels"); if (virSecurityManagerSetAllLabel(driver->securityManager, - vm->def, migratePath) < 0) - goto error; + vm->def, + incoming ? incoming->path : NULL) < 0) + goto cleanup; /* Security manager labeled all devices, therefore - * if any operation from now on fails and we goto cleanup, - * where virSecurityManagerRestoreAllLabel() is called - * (hidden under qemuProcessStop) we need to restore labels. */ - stop_flags &= ~VIR_QEMU_PROCESS_STOP_NO_RELABEL; + * if any operation from now on fails, we need to ask the caller to + * restore labels. + */ + ret = -2; - if (migrateFd != -1) { + if (incoming && incoming->fd != -1) { /* if there's an fd to migrate from, and it's a pipe, put the * proper security label on it */ @@ -4928,43 +4921,44 @@ int qemuProcessStart(virConnectPtr conn, VIR_DEBUG("setting security label on pipe used for migration"); - if (fstat(migrateFd, &stdin_sb) < 0) { + if (fstat(incoming->fd, &stdin_sb) < 0) { virReportSystemError(errno, - _("cannot stat fd %d"), migrateFd); - goto error; + _("cannot stat fd %d"), incoming->fd); + goto cleanup; } if (S_ISFIFO(stdin_sb.st_mode) && - virSecurityManagerSetImageFDLabel(driver->securityManager, vm->def, migrateFd) < 0) - goto error; + virSecurityManagerSetImageFDLabel(driver->securityManager, + vm->def, incoming->fd) < 0) + goto cleanup; } VIR_DEBUG("Labelling done, completing handshake to child"); if (virCommandHandshakeNotify(cmd) < 0) - goto error; + goto cleanup; VIR_DEBUG("Handshake complete, child running"); if (rv == -1) /* The VM failed to start; tear filters before taps */ virDomainConfVMNWFilterTeardown(vm); if (rv == -1) /* The VM failed to start */ - goto error; + goto cleanup; VIR_DEBUG("Setting cgroup for emulator (if required)"); if (qemuSetupCgroupForEmulator(vm) < 0) - goto error; + goto cleanup; VIR_DEBUG("Setting affinity of emulator threads"); if (qemuProcessSetEmulatorAffinity(vm) < 0) - goto error; + goto cleanup; VIR_DEBUG("Waiting for monitor to show up"); if (qemuProcessWaitForMonitor(driver, vm, asyncJob, priv->qemuCaps, pos) < 0) - goto error; + goto cleanup; /* Failure to connect to agent shouldn't be fatal */ if ((rv = qemuConnectAgent(driver, vm)) < 0) { if (rv == -2) - goto error; + goto cleanup; VIR_WARN("Cannot connect to QEMU guest agent for %s", vm->def->name); @@ -4974,50 +4968,50 @@ int qemuProcessStart(virConnectPtr conn, VIR_DEBUG("Detecting if required emulator features are present"); if (!qemuProcessVerifyGuestCPU(driver, vm, asyncJob)) - goto error; + goto cleanup; VIR_DEBUG("Setting up post-init cgroup restrictions"); if (qemuSetupCpusetMems(vm) < 0) - goto error; + goto cleanup; VIR_DEBUG("Detecting VCPU PIDs"); if (qemuProcessDetectVcpuPIDs(driver, vm, asyncJob) < 0) - goto error; + goto cleanup; VIR_DEBUG("Detecting IOThread PIDs"); if (qemuProcessDetectIOThreadPIDs(driver, vm, asyncJob) < 0) - goto error; + goto cleanup; VIR_DEBUG("Setting cgroup for each VCPU (if required)"); if (qemuSetupCgroupForVcpu(vm) < 0) - goto error; + goto cleanup; VIR_DEBUG("Setting cgroup for each IOThread (if required)"); if (qemuSetupCgroupForIOThreads(vm) < 0) - goto error; + goto cleanup; VIR_DEBUG("Setting VCPU affinities"); if (qemuProcessSetVcpuAffinities(vm) < 0) - goto error; + goto cleanup; VIR_DEBUG("Setting affinity of IOThread threads"); if (qemuProcessSetIOThreadsAffinity(vm) < 0) - goto error; + goto cleanup; VIR_DEBUG("Setting scheduler parameters"); if (qemuProcessSetSchedulers(vm) < 0) - goto error; + goto cleanup; VIR_DEBUG("Setting any required VM passwords"); if (qemuProcessInitPasswords(conn, driver, vm, asyncJob) < 0) - goto error; + goto cleanup; /* If we have -device, then addresses are assigned explicitly. * If not, then we have to detect dynamic ones here */ if (!virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DEVICE)) { VIR_DEBUG("Determining domain device PCI addresses"); if (qemuProcessInitPCIAddresses(driver, vm, asyncJob) < 0) - goto error; + goto cleanup; } /* set default link states */ @@ -5025,35 +5019,106 @@ int qemuProcessStart(virConnectPtr conn, * enter the monitor */ VIR_DEBUG("Setting network link states"); if (qemuProcessSetLinkStates(driver, vm, asyncJob) < 0) - goto error; + goto cleanup; VIR_DEBUG("Fetching list of active devices"); if (qemuDomainUpdateDeviceList(driver, vm, asyncJob) < 0) - goto error; + goto cleanup; VIR_DEBUG("Updating info of memory devices"); if (qemuDomainUpdateMemoryDeviceInfo(driver, vm, asyncJob) < 0) - goto error; + goto cleanup; VIR_DEBUG("Setting initial memory amount"); if (qemuProcessSetupBalloon(driver, vm, asyncJob) < 0) - goto error; + goto cleanup; /* Since CPUs were not started yet, the balloon could not return the memory * to the host and thus cur_balloon needs to be updated so that GetXMLdesc * and friends return the correct size in case they can't grab the job */ if (!incoming && !snapshot && qemuProcessRefreshBalloonState(driver, vm, asyncJob) < 0) - goto error; + goto cleanup; VIR_DEBUG("Detecting actual memory size for video device"); if (qemuProcessUpdateVideoRamSize(driver, vm, asyncJob) < 0) - goto error; + goto cleanup; + + if (flags & VIR_QEMU_PROCESS_START_AUTODESTROY && + qemuProcessAutoDestroyAdd(driver, vm, conn) < 0) + goto cleanup; + + ret = 0; + + cleanup: + virCommandFree(cmd); + VIR_FORCE_CLOSE(logfile); + virObjectUnref(cfg); + virObjectUnref(caps); + VIR_FREE(nicindexes); + VIR_FREE(nodeset); + return ret; +} + + +int +qemuProcessStart(virConnectPtr conn, + virQEMUDriverPtr driver, + virDomainObjPtr vm, + qemuDomainAsyncJob asyncJob, + const char *migrateFrom, + int migrateFd, + const char *migratePath, + virDomainSnapshotObjPtr snapshot, + virNetDevVPortProfileOp vmop, + unsigned int flags) +{ + virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver); + qemuDomainObjPrivatePtr priv = vm->privateData; + qemuProcessIncomingDefPtr incoming = NULL; + unsigned int stopFlags; + bool relabel = false; + int ret = -1; + int rv; + + VIR_DEBUG("conn=%p driver=%p vm=%p name=%s id=%d asyncJob=%s " + "migrateFrom=%s migrateFd=%d migratePath=%s " + "snapshot=%p vmop=%d flags=0x%x", + conn, driver, vm, vm->def->name, vm->def->id, + qemuDomainAsyncJobTypeToString(asyncJob), + NULLSTR(migrateFrom), migrateFd, NULLSTR(migratePath), + snapshot, vmop, flags); + + virCheckFlagsGoto(VIR_QEMU_PROCESS_START_COLD | + VIR_QEMU_PROCESS_START_PAUSED | + VIR_QEMU_PROCESS_START_AUTODESTROY, cleanup); + + if ((rv = qemuProcessInit(driver, vm, !!migrateFrom)) < 0) { + if (rv == -2) + goto cleanup; + else + goto stop; + } + + if (migrateFrom) { + incoming = qemuProcessIncomingDefNew(priv->qemuCaps, migrateFrom, + migrateFd, migratePath); + if (!incoming) + goto stop; + } + + if ((rv = qemuProcessLaunch(conn, driver, vm, asyncJob, incoming, + snapshot, vmop, flags)) < 0) { + if (rv == -1) + relabel = true; + goto stop; + } + relabel = true; if (incoming && incoming->deferredURI && qemuMigrationRunIncoming(driver, vm, incoming->deferredURI, asyncJob) < 0) - goto error; + goto stop; if (!(flags & VIR_QEMU_PROCESS_START_PAUSED)) { VIR_DEBUG("Starting domain CPUs"); @@ -5064,7 +5129,7 @@ int qemuProcessStart(virConnectPtr conn, if (virGetLastError() == NULL) virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("resume operation failed")); - goto error; + goto stop; } } else { virDomainObjSetState(vm, VIR_DOMAIN_PAUSED, @@ -5073,19 +5138,14 @@ int qemuProcessStart(virConnectPtr conn, VIR_DOMAIN_PAUSED_USER); } - if (flags & VIR_QEMU_PROCESS_START_AUTODESTROY && - qemuProcessAutoDestroyAdd(driver, vm, conn) < 0) - goto error; - VIR_DEBUG("Writing domain status to disk"); if (virDomainSaveStatus(driver->xmlopt, cfg->stateDir, vm) < 0) - goto error; + goto stop; - /* finally we can call the 'started' hook script if any */ if (qemuProcessStartHook(driver, vm, VIR_HOOK_QEMU_OP_STARTED, VIR_HOOK_SUBOP_BEGIN) < 0) - goto error; + goto stop; /* Keep watching qemu log for errors during incoming migration, otherwise * unset reporting errors from qemu log. */ @@ -5095,20 +5155,17 @@ int qemuProcessStart(virConnectPtr conn, ret = 0; cleanup: - virCommandFree(cmd); - VIR_FORCE_CLOSE(logfile); virObjectUnref(cfg); - virObjectUnref(caps); - VIR_FREE(nicindexes); - VIR_FREE(nodeset); qemuProcessIncomingDefFree(incoming); return ret; - error: - /* We jump here if we failed to start the VM for any reason, or - * if we failed to initialize the now running VM. kill it off and - * pretend we never started it */ - qemuProcessStop(driver, vm, VIR_DOMAIN_SHUTOFF_FAILED, stop_flags); + stop: + stopFlags = 0; + if (!relabel) + stopFlags |= VIR_QEMU_PROCESS_STOP_NO_RELABEL; + if (migrateFrom) + stopFlags |= VIR_QEMU_PROCESS_STOP_MIGRATED; + qemuProcessStop(driver, vm, VIR_DOMAIN_SHUTOFF_FAILED, stopFlags); goto cleanup; } diff --git a/src/qemu/qemu_process.h b/src/qemu/qemu_process.h index 3948caf..54009c5 100644 --- a/src/qemu/qemu_process.h +++ b/src/qemu/qemu_process.h @@ -85,6 +85,15 @@ int qemuProcessInit(virQEMUDriverPtr driver, virDomainObjPtr vm, bool migration); +int qemuProcessLaunch(virConnectPtr conn, + virQEMUDriverPtr driver, + virDomainObjPtr vm, + qemuDomainAsyncJob asyncJob, + qemuProcessIncomingDefPtr incoming, + virDomainSnapshotObjPtr snapshot, + virNetDevVPortProfileOp vmop, + unsigned int flags); + typedef enum { VIR_QEMU_PROCESS_STOP_MIGRATED = 1 << 0, VIR_QEMU_PROCESS_STOP_NO_RELABEL = 1 << 1, -- 2.6.3
On Thu, Nov 19, 2015 at 01:09:16PM +0100, Jiri Denemark wrote:
Once qemuProcessInit was called, qemuProcessLaunch will launch a new QEMU process with stopped virtual CPUs.
Signed-off-by: Jiri Denemark <jdenemar@redhat.com> ---
Notes: Version 2: - save status before running VIR_HOOK_QEMU_OP_STARTED hook - call virSecurityManagerSetAllLabel even if !incoming - move qemuProcessStop out of qemuProcessLaunch to avoid it being called twice
src/qemu/qemu_process.c | 323 ++++++++++++++++++++++++++++-------------------- src/qemu/qemu_process.h | 9 ++ 2 files changed, 199 insertions(+), 133 deletions(-)
ACK, Pavel
Finishes starting a new domain launched by qemuProcessLaunch. Signed-off-by: Jiri Denemark <jdenemar@redhat.com> --- Notes: Version 2: - save status before running VIR_HOOK_QEMU_OP_STARTED hook - rename qemuProcessFinish as qemuProcessFinishStartup - remove unused cfg from qemuProcessStart src/qemu/qemu_process.c | 78 ++++++++++++++++++++++++++++++++----------------- src/qemu/qemu_process.h | 6 ++++ 2 files changed, 57 insertions(+), 27 deletions(-) diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 3066692..4e7ca82 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -5061,6 +5061,52 @@ qemuProcessLaunch(virConnectPtr conn, } +/** + * qemuProcessFinishStartup: + * + * Finish starting a new domain. + */ +int +qemuProcessFinishStartup(virConnectPtr conn, + virQEMUDriverPtr driver, + virDomainObjPtr vm, + bool startCPUs, + virDomainPausedReason pausedReason) +{ + virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver); + int ret = -1; + + if (startCPUs) { + VIR_DEBUG("Starting domain CPUs"); + if (qemuProcessStartCPUs(driver, vm, conn, + VIR_DOMAIN_RUNNING_BOOTED, + QEMU_ASYNC_JOB_NONE) < 0) { + if (!virGetLastError()) + virReportError(VIR_ERR_OPERATION_FAILED, "%s", + _("resume operation failed")); + goto cleanup; + } + } else { + virDomainObjSetState(vm, VIR_DOMAIN_PAUSED, pausedReason); + } + + VIR_DEBUG("Writing domain status to disk"); + if (virDomainSaveStatus(driver->xmlopt, cfg->stateDir, vm) < 0) + goto cleanup; + + if (qemuProcessStartHook(driver, vm, + VIR_HOOK_QEMU_OP_STARTED, + VIR_HOOK_SUBOP_BEGIN) < 0) + goto cleanup; + + ret = 0; + + cleanup: + virObjectUnref(cfg); + return ret; +} + + int qemuProcessStart(virConnectPtr conn, virQEMUDriverPtr driver, @@ -5073,7 +5119,6 @@ qemuProcessStart(virConnectPtr conn, virNetDevVPortProfileOp vmop, unsigned int flags) { - virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver); qemuDomainObjPrivatePtr priv = vm->privateData; qemuProcessIncomingDefPtr incoming = NULL; unsigned int stopFlags; @@ -5120,31 +5165,11 @@ qemuProcessStart(virConnectPtr conn, qemuMigrationRunIncoming(driver, vm, incoming->deferredURI, asyncJob) < 0) goto stop; - if (!(flags & VIR_QEMU_PROCESS_START_PAUSED)) { - VIR_DEBUG("Starting domain CPUs"); - /* Allow the CPUS to start executing */ - if (qemuProcessStartCPUs(driver, vm, conn, - VIR_DOMAIN_RUNNING_BOOTED, - QEMU_ASYNC_JOB_NONE) < 0) { - if (virGetLastError() == NULL) - virReportError(VIR_ERR_INTERNAL_ERROR, - "%s", _("resume operation failed")); - goto stop; - } - } else { - virDomainObjSetState(vm, VIR_DOMAIN_PAUSED, - incoming ? - VIR_DOMAIN_PAUSED_MIGRATION : - VIR_DOMAIN_PAUSED_USER); - } - - VIR_DEBUG("Writing domain status to disk"); - if (virDomainSaveStatus(driver->xmlopt, cfg->stateDir, vm) < 0) - goto stop; - - if (qemuProcessStartHook(driver, vm, - VIR_HOOK_QEMU_OP_STARTED, - VIR_HOOK_SUBOP_BEGIN) < 0) + if (qemuProcessFinishStartup(conn, driver, vm, + !(flags & VIR_QEMU_PROCESS_START_PAUSED), + incoming ? + VIR_DOMAIN_PAUSED_MIGRATION : + VIR_DOMAIN_PAUSED_USER) < 0) goto stop; /* Keep watching qemu log for errors during incoming migration, otherwise @@ -5155,7 +5180,6 @@ qemuProcessStart(virConnectPtr conn, ret = 0; cleanup: - virObjectUnref(cfg); qemuProcessIncomingDefFree(incoming); return ret; diff --git a/src/qemu/qemu_process.h b/src/qemu/qemu_process.h index 54009c5..4f63466 100644 --- a/src/qemu/qemu_process.h +++ b/src/qemu/qemu_process.h @@ -94,6 +94,12 @@ int qemuProcessLaunch(virConnectPtr conn, virNetDevVPortProfileOp vmop, unsigned int flags); +int qemuProcessFinishStartup(virConnectPtr conn, + virQEMUDriverPtr driver, + virDomainObjPtr vm, + bool startCPUs, + virDomainPausedReason pausedReason); + typedef enum { VIR_QEMU_PROCESS_STOP_MIGRATED = 1 << 0, VIR_QEMU_PROCESS_STOP_NO_RELABEL = 1 << 1, -- 2.6.3
On Thu, Nov 19, 2015 at 01:09:17PM +0100, Jiri Denemark wrote:
Finishes starting a new domain launched by qemuProcessLaunch.
Signed-off-by: Jiri Denemark <jdenemar@redhat.com> ---
Notes: Version 2: - save status before running VIR_HOOK_QEMU_OP_STARTED hook - rename qemuProcessFinish as qemuProcessFinishStartup - remove unused cfg from qemuProcessStart
src/qemu/qemu_process.c | 78 ++++++++++++++++++++++++++++++++----------------- src/qemu/qemu_process.h | 6 ++++ 2 files changed, 57 insertions(+), 27 deletions(-)
ACK, Pavel
Signed-off-by: Jiri Denemark <jdenemar@redhat.com> --- Notes: ACKed in version 1 Version 2: - no change src/qemu/qemu_migration.c | 154 +++++++++++++++++++++++++--------------------- 1 file changed, 85 insertions(+), 69 deletions(-) diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index 0c4c94a..4ab6ab7 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -3291,6 +3291,80 @@ qemuMigrationPrepareCleanup(virQEMUDriverPtr driver, qemuDomainObjDiscardAsyncJob(driver, vm); } +static char * +qemuMigrationPrepareIncoming(virDomainObjPtr vm, + bool tunnel, + const char *protocol, + const char *listenAddress, + unsigned short port) +{ + qemuDomainObjPrivatePtr priv = vm->privateData; + char *migrateFrom = NULL; + + if (tunnel) { + if (VIR_STRDUP(migrateFrom, "stdio") < 0) + goto cleanup; + } else { + bool encloseAddress = false; + bool hostIPv6Capable = false; + bool qemuIPv6Capable = false; + struct addrinfo *info = NULL; + struct addrinfo hints = { .ai_flags = AI_ADDRCONFIG, + .ai_socktype = SOCK_STREAM }; + const char *incFormat; + + if (getaddrinfo("::", NULL, &hints, &info) == 0) { + freeaddrinfo(info); + hostIPv6Capable = true; + } + qemuIPv6Capable = virQEMUCapsGet(priv->qemuCaps, + QEMU_CAPS_IPV6_MIGRATION); + + if (listenAddress) { + if (virSocketAddrNumericFamily(listenAddress) == AF_INET6) { + if (!qemuIPv6Capable) { + virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s", + _("qemu isn't capable of IPv6")); + goto cleanup; + } + if (!hostIPv6Capable) { + virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s", + _("host isn't capable of IPv6")); + goto cleanup; + } + /* IPv6 address must be escaped in brackets on the cmd line */ + encloseAddress = true; + } else { + /* listenAddress is a hostname or IPv4 */ + } + } else if (qemuIPv6Capable && hostIPv6Capable) { + /* Listen on :: instead of 0.0.0.0 if QEMU understands it + * and there is at least one IPv6 address configured + */ + listenAddress = "::"; + encloseAddress = true; + } else { + listenAddress = "0.0.0.0"; + } + + /* QEMU will be started with + * -incoming protocol:[<IPv6 addr>]:port, + * -incoming protocol:<IPv4 addr>:port, or + * -incoming protocol:<hostname>:port + */ + if (encloseAddress) + incFormat = "%s:[%s]:%d"; + else + incFormat = "%s:%s:%d"; + if (virAsprintf(&migrateFrom, incFormat, + protocol, listenAddress, port) < 0) + goto cleanup; + } + + cleanup: + return migrateFrom; +} + static int qemuMigrationPrepareAny(virQEMUDriverPtr driver, virConnectPtr dconn, @@ -3397,75 +3471,6 @@ qemuMigrationPrepareAny(virQEMUDriverPtr driver, } } - if (tunnel) { - /* QEMU will be started with -incoming stdio - * (which qemu_command might convert to exec:cat or fd:n) - */ - if (VIR_STRDUP(migrateFrom, "stdio") < 0) - goto cleanup; - } else { - bool encloseAddress = false; - bool hostIPv6Capable = false; - bool qemuIPv6Capable = false; - virQEMUCapsPtr qemuCaps = NULL; - struct addrinfo *info = NULL; - struct addrinfo hints = { .ai_flags = AI_ADDRCONFIG, - .ai_socktype = SOCK_STREAM }; - const char *incFormat; - - if (getaddrinfo("::", NULL, &hints, &info) == 0) { - freeaddrinfo(info); - hostIPv6Capable = true; - } - if (!(qemuCaps = virQEMUCapsCacheLookupCopy(driver->qemuCapsCache, - (*def)->emulator, - (*def)->os.machine))) - goto cleanup; - - qemuIPv6Capable = virQEMUCapsGet(qemuCaps, QEMU_CAPS_IPV6_MIGRATION); - virObjectUnref(qemuCaps); - - if (listenAddress) { - if (virSocketAddrNumericFamily(listenAddress) == AF_INET6) { - if (!qemuIPv6Capable) { - virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s", - _("qemu isn't capable of IPv6")); - goto cleanup; - } - if (!hostIPv6Capable) { - virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s", - _("host isn't capable of IPv6")); - goto cleanup; - } - /* IPv6 address must be escaped in brackets on the cmd line */ - encloseAddress = true; - } else { - /* listenAddress is a hostname or IPv4 */ - } - } else if (qemuIPv6Capable && hostIPv6Capable) { - /* Listen on :: instead of 0.0.0.0 if QEMU understands it - * and there is at least one IPv6 address configured - */ - listenAddress = "::"; - encloseAddress = true; - } else { - listenAddress = "0.0.0.0"; - } - - /* QEMU will be started with - * -incoming protocol:[<IPv6 addr>]:port, - * -incoming protocol:<IPv4 addr>:port, or - * -incoming protocol:<hostname>:port - */ - if (encloseAddress) - incFormat = "%s:[%s]:%d"; - else - incFormat = "%s:%s:%d"; - if (virAsprintf(&migrateFrom, incFormat, - protocol, listenAddress, port) < 0) - goto cleanup; - } - if (!(vm = virDomainObjListAdd(driver->domains, *def, driver->xmlopt, VIR_DOMAIN_OBJ_LIST_ADD_LIVE | @@ -3520,6 +3525,17 @@ qemuMigrationPrepareAny(virQEMUDriverPtr driver, goto endjob; } + virObjectUnref(priv->qemuCaps); + priv->qemuCaps = virQEMUCapsCacheLookupCopy(driver->qemuCapsCache, + vm->def->emulator, + vm->def->os.machine); + if (!priv->qemuCaps) + goto endjob; + + if (!(migrateFrom = qemuMigrationPrepareIncoming(vm, tunnel, protocol, + listenAddress, port))) + goto endjob; + /* Start the QEMU daemon, with the same command-line arguments plus * -incoming $migrateFrom */ -- 2.6.3
Some failure paths in qemuMigrationPrepareAny forgot to kill the just started QEMU process. This patch fixes this by combining 'stop' and 'endjob' label into a new label 'stopjob'. This name was chosen to avoid confusion with the most common semantics of 'endjob'. Normally, 'endjob' is always called at the end of an API to stop the job we entered at the beginning. In qemuMigrationPrepareAny we only want to stop the job in failure path; on success we need to carry the job over to the Finish phase. Signed-off-by: Jiri Denemark <jdenemar@redhat.com> --- Notes: ACKed in version 1 Version 2: - no change src/qemu/qemu_migration.c | 31 ++++++++++++++++--------------- 1 file changed, 16 insertions(+), 15 deletions(-) diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index 4ab6ab7..9a4f19f 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -3522,7 +3522,7 @@ qemuMigrationPrepareAny(virQEMUDriverPtr driver, (pipe(dataFD) < 0 || virSetCloseExec(dataFD[1]) < 0)) { virReportSystemError(errno, "%s", _("cannot create pipe for tunnelled migration")); - goto endjob; + goto stopjob; } virObjectUnref(priv->qemuCaps); @@ -3530,11 +3530,11 @@ qemuMigrationPrepareAny(virQEMUDriverPtr driver, vm->def->emulator, vm->def->os.machine); if (!priv->qemuCaps) - goto endjob; + goto stopjob; if (!(migrateFrom = qemuMigrationPrepareIncoming(vm, tunnel, protocol, listenAddress, port))) - goto endjob; + goto stopjob; /* Start the QEMU daemon, with the same command-line arguments plus * -incoming $migrateFrom @@ -3545,14 +3545,14 @@ qemuMigrationPrepareAny(virQEMUDriverPtr driver, VIR_QEMU_PROCESS_START_PAUSED | VIR_QEMU_PROCESS_START_AUTODESTROY) < 0) { virDomainAuditStart(vm, "migrated", false); - goto endjob; + goto stopjob; } if (tunnel) { if (virFDStreamOpen(st, dataFD[1]) < 0) { virReportSystemError(errno, "%s", _("cannot pass pipe for tunnelled migration")); - goto stop; + goto stopjob; } dataFD[1] = -1; /* 'st' owns the FD now & will close it */ } @@ -3560,17 +3560,17 @@ qemuMigrationPrepareAny(virQEMUDriverPtr driver, if (qemuMigrationSetCompression(driver, vm, flags & VIR_MIGRATE_COMPRESSED, QEMU_ASYNC_JOB_MIGRATION_IN) < 0) - goto stop; + goto stopjob; if (STREQ_NULLABLE(protocol, "rdma") && virProcessSetMaxMemLock(vm->pid, vm->def->mem.hard_limit << 10) < 0) { - goto stop; + goto stopjob; } if (qemuMigrationSetPinAll(driver, vm, flags & VIR_MIGRATE_RDMA_PIN_ALL, QEMU_ASYNC_JOB_MIGRATION_IN) < 0) - goto stop; + goto stopjob; if (mig->lockState) { VIR_DEBUG("Received lockstate %s", mig->lockState); @@ -3593,7 +3593,7 @@ qemuMigrationPrepareAny(virQEMUDriverPtr driver, if (qemuMigrationStartNBDServer(driver, vm, listenAddress, nmigrate_disks, migrate_disks) < 0) { /* error already reported */ - goto endjob; + goto stopjob; } cookieFlags |= QEMU_MIGRATION_COOKIE_NBD; } @@ -3608,7 +3608,7 @@ qemuMigrationPrepareAny(virQEMUDriverPtr driver, } if (qemuDomainCleanupAdd(vm, qemuMigrationPrepareCleanup) < 0) - goto endjob; + goto stopjob; if (!(flags & VIR_MIGRATE_OFFLINE)) { virDomainAuditStart(vm, "migrated", true); @@ -3647,12 +3647,13 @@ qemuMigrationPrepareAny(virQEMUDriverPtr driver, virNWFilterUnlockFilterUpdates(); return ret; - stop: - virDomainAuditStart(vm, "migrated", false); - qemuProcessStop(driver, vm, VIR_DOMAIN_SHUTOFF_FAILED, - VIR_QEMU_PROCESS_STOP_MIGRATED); + stopjob: + if (vm->def->id != -1) { + virDomainAuditStart(vm, "migrated", false); + qemuProcessStop(driver, vm, VIR_DOMAIN_SHUTOFF_FAILED, + VIR_QEMU_PROCESS_STOP_MIGRATED); + } - endjob: qemuMigrationJobFinish(driver, vm); goto cleanup; } -- 2.6.3
On Thu, Nov 19, 2015 at 01:09:19PM +0100, Jiri Denemark wrote:
Some failure paths in qemuMigrationPrepareAny forgot to kill the just started QEMU process. This patch fixes this by combining 'stop' and 'endjob' label into a new label 'stopjob'. This name was chosen to avoid confusion with the most common semantics of 'endjob'. Normally, 'endjob' is always called at the end of an API to stop the job we entered at the beginning. In qemuMigrationPrepareAny we only want to stop the job in failure path; on success we need to carry the job over to the Finish phase.
Signed-off-by: Jiri Denemark <jdenemar@redhat.com> ---
Notes: ACKed in version 1
Version 2: - no change
src/qemu/qemu_migration.c | 31 ++++++++++++++++--------------- 1 file changed, 16 insertions(+), 15 deletions(-)
diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index 4ab6ab7..9a4f19f 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -3522,7 +3522,7 @@ qemuMigrationPrepareAny(virQEMUDriverPtr driver, (pipe(dataFD) < 0 || virSetCloseExec(dataFD[1]) < 0)) { virReportSystemError(errno, "%s", _("cannot create pipe for tunnelled migration")); - goto endjob; + goto stopjob; }
virObjectUnref(priv->qemuCaps); @@ -3530,11 +3530,11 @@ qemuMigrationPrepareAny(virQEMUDriverPtr driver, vm->def->emulator, vm->def->os.machine); if (!priv->qemuCaps) - goto endjob; + goto stopjob;
if (!(migrateFrom = qemuMigrationPrepareIncoming(vm, tunnel, protocol, listenAddress, port))) - goto endjob; + goto stopjob;
/* Start the QEMU daemon, with the same command-line arguments plus * -incoming $migrateFrom @@ -3545,14 +3545,14 @@ qemuMigrationPrepareAny(virQEMUDriverPtr driver, VIR_QEMU_PROCESS_START_PAUSED | VIR_QEMU_PROCESS_START_AUTODESTROY) < 0) { virDomainAuditStart(vm, "migrated", false); - goto endjob; + goto stopjob; }
if (tunnel) { if (virFDStreamOpen(st, dataFD[1]) < 0) { virReportSystemError(errno, "%s", _("cannot pass pipe for tunnelled migration")); - goto stop; + goto stopjob; } dataFD[1] = -1; /* 'st' owns the FD now & will close it */ } @@ -3560,17 +3560,17 @@ qemuMigrationPrepareAny(virQEMUDriverPtr driver, if (qemuMigrationSetCompression(driver, vm, flags & VIR_MIGRATE_COMPRESSED, QEMU_ASYNC_JOB_MIGRATION_IN) < 0) - goto stop; + goto stopjob;
if (STREQ_NULLABLE(protocol, "rdma") && virProcessSetMaxMemLock(vm->pid, vm->def->mem.hard_limit << 10) < 0) { - goto stop; + goto stopjob; }
if (qemuMigrationSetPinAll(driver, vm, flags & VIR_MIGRATE_RDMA_PIN_ALL, QEMU_ASYNC_JOB_MIGRATION_IN) < 0) - goto stop; + goto stopjob;
if (mig->lockState) { VIR_DEBUG("Received lockstate %s", mig->lockState); @@ -3593,7 +3593,7 @@ qemuMigrationPrepareAny(virQEMUDriverPtr driver, if (qemuMigrationStartNBDServer(driver, vm, listenAddress, nmigrate_disks, migrate_disks) < 0) { /* error already reported */ - goto endjob; + goto stopjob; } cookieFlags |= QEMU_MIGRATION_COOKIE_NBD; } @@ -3608,7 +3608,7 @@ qemuMigrationPrepareAny(virQEMUDriverPtr driver, }
if (qemuDomainCleanupAdd(vm, qemuMigrationPrepareCleanup) < 0) - goto endjob; + goto stopjob;
if (!(flags & VIR_MIGRATE_OFFLINE)) { virDomainAuditStart(vm, "migrated", true); @@ -3647,12 +3647,13 @@ qemuMigrationPrepareAny(virQEMUDriverPtr driver, virNWFilterUnlockFilterUpdates(); return ret;
- stop: - virDomainAuditStart(vm, "migrated", false); - qemuProcessStop(driver, vm, VIR_DOMAIN_SHUTOFF_FAILED, - VIR_QEMU_PROCESS_STOP_MIGRATED); + stopjob: + if (vm->def->id != -1) { + virDomainAuditStart(vm, "migrated", false); + qemuProcessStop(driver, vm, VIR_DOMAIN_SHUTOFF_FAILED, + VIR_QEMU_PROCESS_STOP_MIGRATED); + }
- endjob: qemuMigrationJobFinish(driver, vm); goto cleanup;
I wouldn't join those two labels together regarding the change in 1/7 patch. Just change all goto after qemuProcessStart to jump to stop. About the naming we've discussed, what about endmigjob? It's maybe to long, but tels, that we are about to stop the qemuMigrationJob. Pavel
} -- 2.6.3
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On Tue, Nov 24, 2015 at 10:39:20 +0100, Pavel Hrdina wrote:
On Thu, Nov 19, 2015 at 01:09:19PM +0100, Jiri Denemark wrote:
Some failure paths in qemuMigrationPrepareAny forgot to kill the just started QEMU process. This patch fixes this by combining 'stop' and 'endjob' label into a new label 'stopjob'. This name was chosen to avoid confusion with the most common semantics of 'endjob'. Normally, 'endjob' is always called at the end of an API to stop the job we entered at the beginning. In qemuMigrationPrepareAny we only want to stop the job in failure path; on success we need to carry the job over to the Finish phase.
Signed-off-by: Jiri Denemark <jdenemar@redhat.com> ---
Notes: ACKed in version 1
Version 2: - no change
src/qemu/qemu_migration.c | 31 ++++++++++++++++--------------- 1 file changed, 16 insertions(+), 15 deletions(-)
diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index 4ab6ab7..9a4f19f 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c ... @@ -3593,7 +3593,7 @@ qemuMigrationPrepareAny(virQEMUDriverPtr driver, if (qemuMigrationStartNBDServer(driver, vm, listenAddress, nmigrate_disks, migrate_disks) < 0) { /* error already reported */ - goto endjob; + goto stopjob; } cookieFlags |= QEMU_MIGRATION_COOKIE_NBD; } @@ -3608,7 +3608,7 @@ qemuMigrationPrepareAny(virQEMUDriverPtr driver, }
if (qemuDomainCleanupAdd(vm, qemuMigrationPrepareCleanup) < 0) - goto endjob; + goto stopjob;
if (!(flags & VIR_MIGRATE_OFFLINE)) { virDomainAuditStart(vm, "migrated", true); ... I wouldn't join those two labels together regarding the change in 1/7 patch. Just change all goto after qemuProcessStart to jump to stop. About the naming we've discussed, what about endmigjob? It's maybe to long, but tels, that we are about to stop the qemuMigrationJob.
The bug was in the two hunks above. There are two paths going through the code there, one path would need to stop the job and the process while the other path should only stop the job. So instead of having the logic there, I merged the two labels and decide what to do in one place. Jirka
NBD storage migration will not work with offline migration anyway and we already checked that the user did not ask for it. Thus it doesn't make sense to keep the code after 'done' label where we jump in case of offline migration. Signed-off-by: Jiri Denemark <jdenemar@redhat.com> --- Notes: ACKed in version 1 Version 2: - no change src/qemu/qemu_migration.c | 29 +++++++++++++---------------- 1 file changed, 13 insertions(+), 16 deletions(-) diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index 9a4f19f..b6525df 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -3418,6 +3418,9 @@ qemuMigrationPrepareAny(virQEMUDriverPtr driver, "make sense")); goto cleanup; } + cookieFlags = 0; + } else { + cookieFlags = QEMU_MIGRATION_COOKIE_GRAPHICS; } if (!(caps = virQEMUDriverGetCapabilities(driver, false))) @@ -3572,6 +3575,16 @@ qemuMigrationPrepareAny(virQEMUDriverPtr driver, QEMU_ASYNC_JOB_MIGRATION_IN) < 0) goto stopjob; + if (mig->nbd && + flags & (VIR_MIGRATE_NON_SHARED_DISK | VIR_MIGRATE_NON_SHARED_INC) && + virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_NBD_SERVER)) { + if (qemuMigrationStartNBDServer(driver, vm, listenAddress, + nmigrate_disks, migrate_disks) < 0) { + goto stopjob; + } + cookieFlags |= QEMU_MIGRATION_COOKIE_NBD; + } + if (mig->lockState) { VIR_DEBUG("Received lockstate %s", mig->lockState); VIR_FREE(priv->lockState); @@ -3582,22 +3595,6 @@ qemuMigrationPrepareAny(virQEMUDriverPtr driver, } done: - if (flags & VIR_MIGRATE_OFFLINE) - cookieFlags = 0; - else - cookieFlags = QEMU_MIGRATION_COOKIE_GRAPHICS; - - if (mig->nbd && - flags & (VIR_MIGRATE_NON_SHARED_DISK | VIR_MIGRATE_NON_SHARED_INC) && - virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_NBD_SERVER)) { - if (qemuMigrationStartNBDServer(driver, vm, listenAddress, - nmigrate_disks, migrate_disks) < 0) { - /* error already reported */ - goto stopjob; - } - cookieFlags |= QEMU_MIGRATION_COOKIE_NBD; - } - if (qemuMigrationBakeCookie(mig, driver, vm, cookieout, cookieoutlen, cookieFlags) < 0) { /* We could tear down the whole guest here, but -- 2.6.3
Using qemuProcess{Init,Launch,FinishStartup} allows us to run pre-migration commands on destination before asking QEMU to wait for incoming migration data. Signed-off-by: Jiri Denemark <jdenemar@redhat.com> --- src/qemu/qemu_migration.c | 68 ++++++++++++++++++++++++++++++----------------- 1 file changed, 44 insertions(+), 24 deletions(-) diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index b6525df..fe9b1ff 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -3291,14 +3291,16 @@ qemuMigrationPrepareCleanup(virQEMUDriverPtr driver, qemuDomainObjDiscardAsyncJob(driver, vm); } -static char * +static qemuProcessIncomingDefPtr qemuMigrationPrepareIncoming(virDomainObjPtr vm, bool tunnel, const char *protocol, const char *listenAddress, - unsigned short port) + unsigned short port, + int fd) { qemuDomainObjPrivatePtr priv = vm->privateData; + qemuProcessIncomingDefPtr inc = NULL; char *migrateFrom = NULL; if (tunnel) { @@ -3361,8 +3363,11 @@ qemuMigrationPrepareIncoming(virDomainObjPtr vm, goto cleanup; } + inc = qemuProcessIncomingDefNew(priv->qemuCaps, migrateFrom, fd, NULL); + cleanup: - return migrateFrom; + VIR_FREE(migrateFrom); + return inc; } static int @@ -3393,8 +3398,11 @@ qemuMigrationPrepareAny(virQEMUDriverPtr driver, char *xmlout = NULL; unsigned int cookieFlags; virCapsPtr caps = NULL; - char *migrateFrom = NULL; + qemuProcessIncomingDefPtr incoming = NULL; bool taint_hook = false; + bool stopProcess = false; + bool relabel = false; + int rv; virNWFilterReadLockFilterUpdates(); @@ -3528,28 +3536,29 @@ qemuMigrationPrepareAny(virQEMUDriverPtr driver, goto stopjob; } - virObjectUnref(priv->qemuCaps); - priv->qemuCaps = virQEMUCapsCacheLookupCopy(driver->qemuCapsCache, - vm->def->emulator, - vm->def->os.machine); - if (!priv->qemuCaps) + if ((rv = qemuProcessInit(driver, vm, true)) < 0) { + if (rv == -1) + stopProcess = true; goto stopjob; + } + stopProcess = true; - if (!(migrateFrom = qemuMigrationPrepareIncoming(vm, tunnel, protocol, - listenAddress, port))) + if (!(incoming = qemuMigrationPrepareIncoming(vm, tunnel, protocol, + listenAddress, port, + dataFD[0]))) goto stopjob; + dataFD[0] = -1; /* the FD is now owned by incoming */ - /* Start the QEMU daemon, with the same command-line arguments plus - * -incoming $migrateFrom - */ - if (qemuProcessStart(dconn, driver, vm, QEMU_ASYNC_JOB_MIGRATION_IN, - migrateFrom, dataFD[0], NULL, NULL, - VIR_NETDEV_VPORT_PROFILE_OP_MIGRATE_IN_START, - VIR_QEMU_PROCESS_START_PAUSED | - VIR_QEMU_PROCESS_START_AUTODESTROY) < 0) { - virDomainAuditStart(vm, "migrated", false); + rv = qemuProcessLaunch(dconn, driver, vm, QEMU_ASYNC_JOB_MIGRATION_IN, + incoming, NULL, + VIR_NETDEV_VPORT_PROFILE_OP_MIGRATE_IN_START, + VIR_QEMU_PROCESS_START_AUTODESTROY); + if (rv < 0) { + if (rv == -2) + relabel = true; goto stopjob; } + relabel = true; if (tunnel) { if (virFDStreamOpen(st, dataFD[1]) < 0) { @@ -3594,6 +3603,15 @@ qemuMigrationPrepareAny(virQEMUDriverPtr driver, VIR_DEBUG("Received no lockstate"); } + if (incoming->deferredURI && + qemuMigrationRunIncoming(driver, vm, incoming->deferredURI, + QEMU_ASYNC_JOB_MIGRATION_IN) < 0) + goto stopjob; + + if (qemuProcessFinishStartup(dconn, driver, vm, false, + VIR_DOMAIN_PAUSED_MIGRATION) < 0) + goto stopjob; + done: if (qemuMigrationBakeCookie(mig, driver, vm, cookieout, cookieoutlen, cookieFlags) < 0) { @@ -3625,7 +3643,7 @@ qemuMigrationPrepareAny(virQEMUDriverPtr driver, ret = 0; cleanup: - VIR_FREE(migrateFrom); + qemuProcessIncomingDefFree(incoming); VIR_FREE(xmlout); VIR_FORCE_CLOSE(dataFD[0]); VIR_FORCE_CLOSE(dataFD[1]); @@ -3645,10 +3663,12 @@ qemuMigrationPrepareAny(virQEMUDriverPtr driver, return ret; stopjob: - if (vm->def->id != -1) { + if (stopProcess) { + unsigned int stopFlags = VIR_QEMU_PROCESS_STOP_MIGRATED; + if (!relabel) + stopFlags |= VIR_QEMU_PROCESS_STOP_NO_RELABEL; virDomainAuditStart(vm, "migrated", false); - qemuProcessStop(driver, vm, VIR_DOMAIN_SHUTOFF_FAILED, - VIR_QEMU_PROCESS_STOP_MIGRATED); + qemuProcessStop(driver, vm, VIR_DOMAIN_SHUTOFF_FAILED, stopFlags); } qemuMigrationJobFinish(driver, vm); -- 2.6.3
On Thu, Nov 19, 2015 at 01:09:21PM +0100, Jiri Denemark wrote:
Using qemuProcess{Init,Launch,FinishStartup} allows us to run pre-migration commands on destination before asking QEMU to wait for incoming migration data.
Signed-off-by: Jiri Denemark <jdenemar@redhat.com> --- src/qemu/qemu_migration.c | 68 ++++++++++++++++++++++++++++++----------------- 1 file changed, 44 insertions(+), 24 deletions(-)
diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index b6525df..fe9b1ff 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -3291,14 +3291,16 @@ qemuMigrationPrepareCleanup(virQEMUDriverPtr driver, qemuDomainObjDiscardAsyncJob(driver, vm); }
-static char * +static qemuProcessIncomingDefPtr qemuMigrationPrepareIncoming(virDomainObjPtr vm, bool tunnel, const char *protocol, const char *listenAddress, - unsigned short port) + unsigned short port, + int fd) { qemuDomainObjPrivatePtr priv = vm->privateData; + qemuProcessIncomingDefPtr inc = NULL; char *migrateFrom = NULL;
if (tunnel) { @@ -3361,8 +3363,11 @@ qemuMigrationPrepareIncoming(virDomainObjPtr vm, goto cleanup; }
+ inc = qemuProcessIncomingDefNew(priv->qemuCaps, migrateFrom, fd, NULL); + cleanup: - return migrateFrom; + VIR_FREE(migrateFrom); + return inc; }
static int @@ -3393,8 +3398,11 @@ qemuMigrationPrepareAny(virQEMUDriverPtr driver, char *xmlout = NULL; unsigned int cookieFlags; virCapsPtr caps = NULL; - char *migrateFrom = NULL; + qemuProcessIncomingDefPtr incoming = NULL; bool taint_hook = false; + bool stopProcess = false; + bool relabel = false; + int rv;
virNWFilterReadLockFilterUpdates();
@@ -3528,28 +3536,29 @@ qemuMigrationPrepareAny(virQEMUDriverPtr driver, goto stopjob; }
- virObjectUnref(priv->qemuCaps); - priv->qemuCaps = virQEMUCapsCacheLookupCopy(driver->qemuCapsCache, - vm->def->emulator, - vm->def->os.machine); - if (!priv->qemuCaps) + if ((rv = qemuProcessInit(driver, vm, true)) < 0) { + if (rv == -1) + stopProcess = true; goto stopjob; + } + stopProcess = true;
With the changes in 1/7 and 5/7 this could be simple: if (qemuProcessInit(driver, vm, true) < 0) goto endjob/endmigjob/stobjob; And every other goto will jump to 'stop' to make sure that the qemuProcess is stopped too.
- if (!(migrateFrom = qemuMigrationPrepareIncoming(vm, tunnel, protocol, - listenAddress, port))) + if (!(incoming = qemuMigrationPrepareIncoming(vm, tunnel, protocol, + listenAddress, port, + dataFD[0]))) goto stopjob; + dataFD[0] = -1; /* the FD is now owned by incoming */
- /* Start the QEMU daemon, with the same command-line arguments plus - * -incoming $migrateFrom - */ - if (qemuProcessStart(dconn, driver, vm, QEMU_ASYNC_JOB_MIGRATION_IN, - migrateFrom, dataFD[0], NULL, NULL, - VIR_NETDEV_VPORT_PROFILE_OP_MIGRATE_IN_START, - VIR_QEMU_PROCESS_START_PAUSED | - VIR_QEMU_PROCESS_START_AUTODESTROY) < 0) { - virDomainAuditStart(vm, "migrated", false); + rv = qemuProcessLaunch(dconn, driver, vm, QEMU_ASYNC_JOB_MIGRATION_IN, + incoming, NULL, + VIR_NETDEV_VPORT_PROFILE_OP_MIGRATE_IN_START, + VIR_QEMU_PROCESS_START_AUTODESTROY); + if (rv < 0) { + if (rv == -2) + relabel = true; goto stopjob; } + relabel = true;
if (tunnel) { if (virFDStreamOpen(st, dataFD[1]) < 0) {
[...] Otherwise seems ok, Pavel
participants (2)
-
Jiri Denemark -
Pavel Hrdina