Re: [libvirt] [PATCH] RFC: Support QEMU live uprgade

On 11/12/2013 05:14 AM, Zheng Sheng ZS Zhou wrote: 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). I know this is an RFC patch, but ideally the final patch will be split into parts. Part 1: public interface (the files above). Part 4: Qemu driver implementation (these 3 files) Part 3: RPC implementation (these 2) 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. 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(). Indentation is off. Which flags? 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. 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. Wrong. The earliest this can be introduced is 1.1.5, not 1.1.3. Indentation is off. { belongs on its own line when starting a function. Speak like Yoda we do not. Put your constants on the right side of ==. Function bodies start with { on own line. (Several more instances) Computing the socket name once... ...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? Spaces around operators, and for loops prefer post-decrement rather than pre-decrement: for (i = 0; i < VIR_UUID_BUFLEN; 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). In general, we tend to omit {} around one-liner bodies (not mandatory to omit, and different from qemu's style where {} is always required). Another poorly-formatted for loop that should use memcpy instead. 1.1.5 at the earliest. 1.1.5 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). 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? 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