Re: [libvirt] [PATCH 1/3] qemu: handle new 'setup' migration state

On 07/26/2013 11:47 AM, mrhines(a)linux.vnet.ibm.com wrote: > From: "Michael R. Hines" <mrhines(a)us.ibm.com&gt; > > Previously, QEMU's 'setup' state was no a formal state in their > state machine, but it is now. This state is used by RDMA to optionally > perform memory pinning. This state is now exposed over the monitor > and also measured in the migration info status. > > This patch consumes both the new setup state as well as the timestamp > of the total time spent in that state as reported by QEMU. > > RDMA migrations perform an optional 'pin-all' operation du > > Signed-off-by: Michael R. Hines <mrhines(a)us.ibm.com&gt; > --- > include/libvirt/libvirt.h.in | 2 ++ > src/qemu/qemu_migration.c | 6 ++++++ > src/qemu/qemu_monitor.c | 2 +- > src/qemu/qemu_monitor.h | 11 +++++++++++ > src/qemu/qemu_monitor_json.c | 18 ++++++++++++++++++ > 5 files changed, 38 insertions(+), 1 deletion(-) > > diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in > index c0eb25b..31fb37e 100644 > --- a/include/libvirt/libvirt.h.in > +++ b/include/libvirt/libvirt.h.in > @@ -4048,6 +4048,8 @@ struct _virDomainJobInfo { > /* Time is measured in mill-seconds */ > unsigned long long timeElapsed; /* Always set */ > unsigned long long timeRemaining; /* Only for VIR_DOMAIN_JOB_BOUNDED */ > + unsigned long long setupTime; /* length of the SETUP phase */ > + double mbps; /* Migration throughput in Mbps */ This series adds a new feature, and we're already in freeze for 1.1.1, so it would have to be post-freeze.

NACK; this is a change to ABI. We cannot change the existing struct size without invalidating apps compiled against an earlier version of the library (ie. virDomainGetJobInfo is cast in stone). Rather, we should be adding new VIR_DOMAIN_JOB_* macros for use in the virTypedParameters of virDomainGetJobStats().

It does point out a bug that can be fixed during freeze: we should fix our documentation so that virDomainGetJobInfo() refers to the better virDomainGetJobStats(). Somewhat related: I mentioned in another thread that neither virDomainGetJob{Info,Stats} nor virDomainGetBlockJobInfo play nicely with the idea of being able to have multiple jobs running in parallel, which is necessary if we want to be able to support fleecing of point-in-time snapshots from multiple clients. Ultimately, that means I think we need to enhance the job mechanism so that new jobs return a handle, then we add new API that can query and abort jobs based on the handle instead of being limited to only the current job; I guess for back-compat, it would also help to have an API that sets the current job out of a set of jobs so that the old APIs are still usable. If we do an upgrade of the job API, we would make sure that the new query method sticks to virTypedParameters in the same was as virDomainGetJobStats().