On 11/12/2013 05:14 AM, Zheng Sheng ZS Zhou wrote:
>>From 2b659584f2cbe676c843ddeaf198c9a8368ff0ff Mon Sep 17 00:00:00 2001
> From: Zhou Zheng Sheng <zhshzhou@xxxxxxxxxxxxxxxxxx>
> Date: Wed, 30 Oct 2013 15:36:49 +0800
> Subject: [PATCH] RFC: Support QEMU live uprgade
>
> This patch is to support upgrading QEMU version without restarting the
> virtual machine.
>
> Add new API virDomainQemuLiveUpgrade(), and a new virsh command
> qemu-live-upgrade. virDomainQemuLiveUpgrade() migrates a running VM to
> the same host as a new VM with new name and new UUID. Then it shutdown
> the original VM and drop the new VM definition without shutdown the QEMU
> process of the new VM. At last it attaches original VM to the new QEMU
> process.
>
> Firstly the admin installs new QEMU package, then he runs
> virsh qemu-live-upgrade domain_name
> to trigger our virDomainQemuLiveUpgrade() upgrading flow.
In general, I agree that we need a new API (in fact, I think I helped
suggest why we need it as opposed to reusing existing migration API,
precisely for some of the deadlock reasons you called out in your reply
to Dan). But the new API should still reuse as much of the existing
migration code as possible (refactor that to be reusable, rather than
bulk copying into completely new code). High-level review below (I
didn't test whether things work or look for details like memory leaks,
so much as a first impression of style problems and even some major
design problems).
>
> Signed-off-by: Zhou Zheng Sheng <zhshzhou@xxxxxxxxxxxxxxxxxx>
> ---
> include/libvirt/libvirt.h.in | 3 +
> src/driver.h | 4 +
> src/libvirt.c | 23 +++
> src/libvirt_public.syms | 1 +
I know this is an RFC patch, but ideally the final patch will be split
into parts. Part 1: public interface (the files above).
> src/qemu/qemu_driver.c | 339 +++++++++++++++++++++++++++++++++++++++++++
> src/qemu/qemu_migration.c | 2 +-
> src/qemu/qemu_migration.h | 3 +
Part 4: Qemu driver implementation (these 3 files)
> src/remote/remote_driver.c | 1 +
> src/remote/remote_protocol.x | 19 ++-
Part 3: RPC implementation (these 2)
> tools/virsh-domain.c | 139 ++++++++++++++++++
Part 2: Use of the public implementation (should compile and gracefully
fail until parts 3 and 4 are also applied; doing it early means you can
test the error paths as well as validate that the C API seems usable)
You may also need to do a Part 5 to modify the python bindings,
depending on whether the code generator was able to get it working on
your behalf.
> +++ b/include/libvirt/libvirt.h.in
> @@ -1331,6 +1331,9 @@ int virDomainMigrateGetMaxSpeed(virDomainPtr domain,
> unsigned long *bandwidth,
> unsigned int flags);
>
> +virDomainPtr virDomainQemuLiveUpgrade(virDomainPtr domain,
> + unsigned int flags);
No bandwidth parameter? Seems inconsistent with all the other migration
APIs. Also, since this is moving from one qemu process to another, this
_totally_ seems like a case for allowing a destination XML argument
(lots of people have asked for the ability to point qemu to an alternate
disk source with identical bits visible to the guest but better
characteristics for the host; right now, their solution is to 'virsh
save', 'virsh save-image-edit', 'virsh restore'; but your API could let
the specify an alternate XML and do a single live-upgrade so that the
new qemu is using the new file name).
Anything with "Qemu" in the API name belongs in libvirt-qemu.h. But
this API seems like it is useful to more than just qemu; it could be
used for other hypervisors as well. You need a more generic name; mayb
virDomainLiveUpgrade().
> +++ b/src/driver.h
> @@ -686,6 +686,9 @@ typedef int
> const char *args,
> char ***models,
> unsigned int flags);
> +typedef virDomainPtr
> +(*virDrvDomainQemuLiveUpgrade)(virDomainPtr domain,
> + unsigned int flags);
Indentation is off.
> +++ b/src/libvirt.c
> @@ -7524,6 +7524,29 @@ error:
>
>
> /**
> + * virDomainQemuLiveUpgrade:
> + * @domain: a domain object
> + * @flags: bitwise-OR of flags
Which flags?
> + *
> + * Live upgrade qemu binary version of the domain.
> + *
> + * Returns the new domain object if the upgrade was successful,
> + * or NULL in case of error.
Unusual spacing; indenting by more than one space causes the generated
.html documentation to treat that line as code rather than prose. You
need to be explicit that if live upgrade fails, the domain is LOST (in
contrast to normal migration, where failure just reverts back to running
on the source). Users must be aware of the risks.
> + */
> +virDomainPtr
> +virDomainQemuLiveUpgrade(virDomainPtr domain,
> + unsigned int flags)
> +{
> + VIR_DEBUG("domain=%p, flags=%x", domain, flags);
> + if (!domain->conn->driver->domainQemuLiveUpgrade) {
> + virLibConnError(VIR_ERR_INTERNAL_ERROR, __FUNCTION__);
> + return NULL;
> + }
> + return domain->conn->driver->domainQemuLiveUpgrade(domain, flags);
Doesn't look like most other API calls. In particular, missing a
VIR_DOMAIN_DEBUG for logging the call, missing a virResetLastError() for
correct error handling, missing a check against invalid domain, missing
a check against read-only connections, etc.
> +++ b/src/libvirt_public.syms
> @@ -637,6 +637,7 @@ LIBVIRT_1.1.1 {
> LIBVIRT_1.1.3 {
> global:
> virConnectGetCPUModelNames;
> + virDomainQemuLiveUpgrade;
> } LIBVIRT_1.1.1;
Wrong. The earliest this can be introduced is 1.1.5, not 1.1.3.
>
> +static virDomainDefPtr
> +virDomainDefLiveCopy(virDomainDefPtr src,
> + virCapsPtr caps,
> + virDomainXMLOptionPtr xmlopt)
Indentation is off.
> +
> +static virDomainDefPtr
> +qemuLiveUpgradeMiniBegin(virQEMUDriverPtr driver, virDomainObjPtr vm) {
{ belongs on its own line when starting a function.
> + if (-1 == virUUIDGenerate(newDef->uuid))
Speak like Yoda we do not. Put your constants on the right side of ==.
> +
> +static virDomainObjPtr
> +qemuLiveUpgradeMiniPrepare(virQEMUDriverPtr driver, virConnectPtr conn,
> + virDomainDefPtr newDef) {
Function bodies start with { on own line. (Several more instances)
> + virDomainObjPtr newVm = NULL;
> + virDomainObjPtr result = NULL;
> + char *upgradeUri = NULL;
> + virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver);
> +
> + newVm = virDomainObjListAdd(driver->domains, newDef, driver->xmlopt,
> + VIR_DOMAIN_OBJ_LIST_ADD_LIVE |
> + VIR_DOMAIN_OBJ_LIST_ADD_CHECK_LIVE, NULL);
> + if (!newVm)
> + goto cleanup;
> + newDef = NULL;
> +
> + if (virAsprintf(&upgradeUri, "unix:%s/qemu.live.upgrade.%s.sock",
> + cfg->libDir, newVm->def->name) < 0)
Computing the socket name once...
> +static bool
> +qemuLiveUpgradeMiniPerform(virQEMUDriverPtr driver, virDomainObjPtr vm,
> + const char *newName) {
> + char *upgradeSock = NULL;
> + qemuDomainObjPrivatePtr priv = vm->privateData;
> + bool result = false;
> + bool migrate = false;
> + int r = 0;
> + virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver);
> +
> + if (virAsprintf(&upgradeSock, "%s/qemu.live.upgrade.%s.sock",
> + cfg->libDir, newName) < 0)
...and again. Seems error prone if we update one string but not the
other; shouldn't the name be generated by a common helper function?
> +
> + for (i=0; i < VIR_UUID_BUFLEN; ++i) {
Spaces around operators, and for loops prefer post-decrement rather than
pre-decrement:
for (i = 0; i < VIR_UUID_BUFLEN; i++)
> + tmpDef->uuid[i] = origUuid[i];
For that matter, a for loop is rather inefficient; just
memcpy(tmpDef->uuid, origUuid, VIR_UUID_BUFLEN) (supposing that using a
temporary uuid even makes sense design-wise; after all, the uuid you
pass on the destination qemu command line is guest-visible via dmidecode
in the guest; you must NOT start the guest with a different uuid and
then fix it up later in the host).
> + if (!newVm) {
> + goto cleanup;
> + }
> +
> + if (!qemuLiveUpgradeMiniPerform(driver, vm, newVm->def->name)) {
> + goto cleanup;
> + }
In general, we tend to omit {} around one-liner bodies (not mandatory to
omit, and different from qemu's style where {} is always required).
> +
> + if (VIR_STRDUP(origName, vm->def->name) < 0)
> + goto cleanup;
> + for (i=0; i < VIR_UUID_BUFLEN; ++i) {
> + origUuid[i] = vm->def->uuid[i];
> + }
Another poorly-formatted for loop that should use memcpy instead.
> @@ -15892,6 +16230,7 @@ static virDriver qemuDriver = {
> .domainMigrateFinish3Params = qemuDomainMigrateFinish3Params, /* 1.1.0 */
> .domainMigrateConfirm3Params = qemuDomainMigrateConfirm3Params, /* 1.1.0 */
> .connectGetCPUModelNames = qemuConnectGetCPUModelNames, /* 1.1.3 */
> + .domainQemuLiveUpgrade = qemuDomainQemuLiveUpgrade, /* 1.1.3 */
1.1.5 at the earliest.
> +++ b/src/remote/remote_driver.c
> @@ -7013,6 +7013,7 @@ static virDriver remote_driver = {
> .domainMigrateFinish3Params = remoteDomainMigrateFinish3Params, /* 1.1.0 */
> .domainMigrateConfirm3Params = remoteDomainMigrateConfirm3Params, /* 1.1.0 */
> .connectGetCPUModelNames = remoteConnectGetCPUModelNames, /* 1.1.3 */
> + .domainQemuLiveUpgrade = remoteDomainQemuLiveUpgrade, /* 1.1.3 */
1.1.5
> +++ b/src/remote/remote_protocol.x
> @@ -2849,6 +2849,15 @@ struct remote_connect_get_cpu_model_names_ret {
> int ret;
> };
>
> +struct remote_domain_qemu_live_upgrade_args {
> + remote_nonnull_domain dom;
> + unsigned int flags;
Needs to match the final API we decide on (again, I don't like qemu in
the name, since I think this is more generic than libvirt-qemu.so; I
also think you need at least destination xml and probably also bandwidth
arguments for consistency; even a virTypedParameter argument will save
you hassle if down the road we think of more items that need
parameterization).
> +static void
> +vshQemuLiveUpgradeTimeout(vshControl *ctl,
> + virDomainPtr dom,
> + void *opaque ATTRIBUTE_UNUSED)
> +{
> + vshDebug(ctl, VSH_ERR_DEBUG, "suspending the domain, "
> + "since QEMU live uprgade timed out\n");
Is upgrade timeout even possible with live upgrade, or is the point of
page-flipping on a local machine to be that we are fast enough to not
have to throttle things and never need to worry about converging in a
timely manner?
> @@ -10796,6 +10929,12 @@ const vshCmdDef domManagementCmds[] = {
> .info = info_qemu_agent_command,
> .flags = 0
> },
> + {.name = "qemu-live-upgrade",
If this does not live in libvirt-qemu.so, then the virsh command must
not have 'qemu-' in the name.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list