[libvirt] [PATCH 0/5] Introduce virDomainMigrateSetDowntime API
This API call is supposed to change maximum tolerable downtime for live migrations. Jiri Denemark (5): Internal driver API for virDomainMigrateSetDowntime Public virDomainMigrateSetDowntime API Implement virDomainMigrateSetDowntime in remote driver Wire protocol and dispatcher for virDomainMigrateSetDowntime Implement virDomainMigrateSetDowntime in qemu driver daemon/remote.c | 29 ++++++++++ daemon/remote_dispatch_args.h | 1 + daemon/remote_dispatch_prototypes.h | 8 +++ daemon/remote_dispatch_table.h | 5 ++ include/libvirt/libvirt.h.in | 3 + src/driver.h | 5 ++ src/esx/esx_driver.c | 1 + src/libvirt.c | 46 ++++++++++++++++ src/libvirt_public.syms | 5 ++ src/lxc/lxc_driver.c | 1 + src/opennebula/one_driver.c | 1 + src/openvz/openvz_driver.c | 1 + src/phyp/phyp_driver.c | 1 + src/qemu/qemu_driver.c | 47 +++++++++++++++++ src/qemu/qemu_monitor.c | 15 +++++ src/qemu/qemu_monitor.h | 3 + src/qemu/qemu_monitor_json.c | 29 ++++++++++ src/qemu/qemu_monitor_json.h | 3 + src/qemu/qemu_monitor_text.c | 27 ++++++++++ src/qemu/qemu_monitor_text.h | 3 + src/remote/remote_driver.c | 27 ++++++++++ src/remote/remote_protocol.c | 11 ++++ src/remote/remote_protocol.h | 97 +++++++++++++++++++---------------- src/remote/remote_protocol.x | 9 +++- src/test/test_driver.c | 1 + src/uml/uml_driver.c | 1 + src/vbox/vbox_tmpl.c | 1 + src/xen/xen_driver.c | 1 + src/xenapi/xenapi_driver.c | 1 + 29 files changed, 338 insertions(+), 45 deletions(-)
Signed-off-by: Jiri Denemark <jdenemar@redhat.com> --- src/driver.h | 5 +++++ src/esx/esx_driver.c | 1 + src/lxc/lxc_driver.c | 1 + src/opennebula/one_driver.c | 1 + src/openvz/openvz_driver.c | 1 + src/phyp/phyp_driver.c | 1 + src/qemu/qemu_driver.c | 1 + src/remote/remote_driver.c | 1 + src/test/test_driver.c | 1 + src/uml/uml_driver.c | 1 + src/vbox/vbox_tmpl.c | 1 + src/xen/xen_driver.c | 1 + src/xenapi/xenapi_driver.c | 1 + 13 files changed, 17 insertions(+), 0 deletions(-) diff --git a/src/driver.h b/src/driver.h index a64bba0..e48eacd 100644 --- a/src/driver.h +++ b/src/driver.h @@ -381,6 +381,10 @@ typedef int typedef int (*virDrvDomainAbortJob)(virDomainPtr domain); +typedef int + (*virDrvDomainMigrateSetDowntime)(virDomainPtr domain, + unsigned long long downtime); + /** * _virDriver: * @@ -474,6 +478,7 @@ struct _virDriver { virDrvCPUBaseline cpuBaseline; virDrvDomainGetJobInfo domainGetJobInfo; virDrvDomainAbortJob domainAbortJob; + virDrvDomainMigrateSetDowntime domainMigrateSetDowntime; }; typedef int diff --git a/src/esx/esx_driver.c b/src/esx/esx_driver.c index c47af1c..8450ceb 100644 --- a/src/esx/esx_driver.c +++ b/src/esx/esx_driver.c @@ -3410,6 +3410,7 @@ static virDriver esxDriver = { NULL, /* cpuBaseline */ NULL, /* domainGetJobInfo */ NULL, /* domainAbortJob */ + NULL, /* domainMigrateSetDowntime */ }; diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c index 35c8659..fe918f0 100644 --- a/src/lxc/lxc_driver.c +++ b/src/lxc/lxc_driver.c @@ -2453,6 +2453,7 @@ static virDriver lxcDriver = { NULL, /* cpuBaseline */ NULL, /* domainGetJobInfo */ NULL, /* domainAbortJob */ + NULL, /* domainMigrateSetDowntime */ }; static virStateDriver lxcStateDriver = { diff --git a/src/opennebula/one_driver.c b/src/opennebula/one_driver.c index 9fc0ada..3bc617e 100644 --- a/src/opennebula/one_driver.c +++ b/src/opennebula/one_driver.c @@ -788,6 +788,7 @@ static virDriver oneDriver = { NULL, /* cpuBaseline */ NULL, /* domainGetJobInfo */ NULL, /* domainAbortJob */ + NULL, /* domainMigrateSetDowntime */ }; static virStateDriver oneStateDriver = { diff --git a/src/openvz/openvz_driver.c b/src/openvz/openvz_driver.c index b7bc43b..2b3978c 100644 --- a/src/openvz/openvz_driver.c +++ b/src/openvz/openvz_driver.c @@ -1540,6 +1540,7 @@ static virDriver openvzDriver = { NULL, /* cpuBaseline */ NULL, /* domainGetJobInfo */ NULL, /* domainAbortJob */ + NULL, /* domainMigrateSetDowntime */ }; int openvzRegister(void) { diff --git a/src/phyp/phyp_driver.c b/src/phyp/phyp_driver.c index 32c0a7a..3a96c51 100644 --- a/src/phyp/phyp_driver.c +++ b/src/phyp/phyp_driver.c @@ -1657,6 +1657,7 @@ virDriver phypDriver = { NULL, /* cpuBaseline */ NULL, /* domainGetJobInfo */ NULL, /* domainAbortJob */ + NULL, /* domainMigrateSetDowntime */ }; int diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 5b2c26a..8527c96 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -9580,6 +9580,7 @@ static virDriver qemuDriver = { qemuCPUBaseline, /* cpuBaseline */ qemuDomainGetJobInfo, /* domainGetJobInfo */ qemuDomainAbortJob, /* domainAbortJob */ + NULL, /* domainMigrateSetDowntime */ }; diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c index 11513bd..e654610 100644 --- a/src/remote/remote_driver.c +++ b/src/remote/remote_driver.c @@ -9126,6 +9126,7 @@ static virDriver remote_driver = { remoteCPUBaseline, /* cpuBaseline */ remoteDomainGetJobInfo, /* domainGetJobInfo */ remoteDomainAbortJob, /* domainFinishJob */ + NULL, /* domainMigrateSetDowntime */ }; static virNetworkDriver network_driver = { diff --git a/src/test/test_driver.c b/src/test/test_driver.c index 7960812..f885b34 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -5245,6 +5245,7 @@ static virDriver testDriver = { NULL, /* cpuBaseline */ NULL, /* domainGetJobInfo */ NULL, /* domainAbortJob */ + NULL, /* domainMigrateSetDowntime */ }; static virNetworkDriver testNetworkDriver = { diff --git a/src/uml/uml_driver.c b/src/uml/uml_driver.c index eec239f..51784a5 100644 --- a/src/uml/uml_driver.c +++ b/src/uml/uml_driver.c @@ -1932,6 +1932,7 @@ static virDriver umlDriver = { NULL, /* cpuBaseline */ NULL, /* domainGetJobInfo */ NULL, /* domainAbortJob */ + NULL, /* domainMigrateSetDowntime */ }; diff --git a/src/vbox/vbox_tmpl.c b/src/vbox/vbox_tmpl.c index b808910..da5c141 100644 --- a/src/vbox/vbox_tmpl.c +++ b/src/vbox/vbox_tmpl.c @@ -7066,6 +7066,7 @@ virDriver NAME(Driver) = { NULL, /* cpuBaseline */ NULL, /* domainGetJobInfo */ NULL, /* domainAbortJob */ + NULL, /* domainMigrateSetDowntime */ }; virNetworkDriver NAME(NetworkDriver) = { diff --git a/src/xen/xen_driver.c b/src/xen/xen_driver.c index 5b9649c..f811095 100644 --- a/src/xen/xen_driver.c +++ b/src/xen/xen_driver.c @@ -1906,6 +1906,7 @@ static virDriver xenUnifiedDriver = { NULL, /* cpuBaseline */ NULL, /* domainGetJobInfo */ NULL, /* domainAbortJob */ + NULL, /* domainMigrateSetDowntime */ }; /** diff --git a/src/xenapi/xenapi_driver.c b/src/xenapi/xenapi_driver.c index ad77068..060584d 100644 --- a/src/xenapi/xenapi_driver.c +++ b/src/xenapi/xenapi_driver.c @@ -1693,6 +1693,7 @@ static virDriver xenapiDriver = { NULL, /* cpuBaseline */ NULL, /* domainGetJobInfo */ NULL, /* domainAbortJob */ + NULL, /* domainMigrateSetDowntime */ }; /** -- 1.7.0.2
Signed-off-by: Jiri Denemark <jdenemar@redhat.com> --- include/libvirt/libvirt.h.in | 3 ++ src/libvirt.c | 46 ++++++++++++++++++++++++++++++++++++++++++ src/libvirt_public.syms | 5 ++++ 3 files changed, 54 insertions(+), 0 deletions(-) diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in index 0d1b5b5..eafab40 100644 --- a/include/libvirt/libvirt.h.in +++ b/include/libvirt/libvirt.h.in @@ -408,6 +408,9 @@ int virDomainMigrateToURI (virDomainPtr domain, const char *duri, unsigned long flags, const char *dname, unsigned long bandwidth); +int virDomainMigrateSetDowntime (virDomainPtr domain, + unsigned long long downtime); + /** * VIR_NODEINFO_MAXCPUS: * @nodeinfo: virNodeInfo instance diff --git a/src/libvirt.c b/src/libvirt.c index 1d9b878..c73caa3 100644 --- a/src/libvirt.c +++ b/src/libvirt.c @@ -11265,3 +11265,49 @@ error: virDispatchError(conn); return -1; } + + +/** + * virDomainMigrateSetDowntime: + * @domain: a domain object + * @downtime: maximum tolerable downtime for live migration, in nanoseconds + * + * Sets maximum tolerable time for which the domain is allowed to be paused + * at the end of live migration. + * + * Returns 0 in case of success, -1 otherwise. + */ +int +virDomainMigrateSetDowntime(virDomainPtr domain, + unsigned long long downtime) +{ + virConnectPtr conn; + + DEBUG("domain=%p, downtime=%llu", domain, downtime); + + virResetLastError(); + + if (!VIR_IS_CONNECTED_DOMAIN(domain)) { + virLibDomainError(NULL, VIR_ERR_INVALID_DOMAIN, __FUNCTION__); + virDispatchError(NULL); + return -1; + } + + conn = domain->conn; + if (conn->flags & VIR_CONNECT_RO) { + virLibDomainError(domain, VIR_ERR_OPERATION_DENIED, __FUNCTION__); + goto error; + } + + if (conn->driver->domainMigrateSetDowntime) { + if (conn->driver->domainMigrateSetDowntime(domain, downtime) < 0) + goto error; + return 0; + } + + virLibConnError(conn, VIR_ERR_NO_SUPPORT, __FUNCTION__); + +error: + virDispatchError(conn); + return -1; +} diff --git a/src/libvirt_public.syms b/src/libvirt_public.syms index 64e7505..9d9f0c2 100644 --- a/src/libvirt_public.syms +++ b/src/libvirt_public.syms @@ -358,4 +358,9 @@ LIBVIRT_0.7.7 { virDomainAbortJob; } LIBVIRT_0.7.5; +LIBVIRT_0.7.8 { + global: + virDomainMigrateSetDowntime; +} LIBVIRT_0.7.7; + # .... define new API here using predicted next version number .... -- 1.7.0.2
Signed-off-by: Jiri Denemark <jdenemar@redhat.com> --- src/remote/remote_driver.c | 28 +++++++++++++++++++++++++++- 1 files changed, 27 insertions(+), 1 deletions(-) diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c index e654610..00158a6 100644 --- a/src/remote/remote_driver.c +++ b/src/remote/remote_driver.c @@ -7707,6 +7707,32 @@ done: } +static int +remoteDomainMigrateSetDowntime(virDomainPtr domain, + unsigned long long downtime) +{ + struct private_data *priv = domain->conn->privateData; + remote_domain_migrate_set_downtime_args args; + int rv = -1; + + remoteDriverLock(priv); + + make_nonnull_domain(&args.dom, domain); + args.downtime = downtime; + + if (call(domain->conn, priv, 0, REMOTE_PROC_DOMAIN_MIGRATE_SET_DOWNTIME, + (xdrproc_t) xdr_remote_domain_migrate_set_downtime_args, (char *) &args, + (xdrproc_t) xdr_void, (char *) NULL) == -1) + goto done; + + rv = 0; + +done: + remoteDriverUnlock(priv); + return rv; +} + + /*----------------------------------------------------------------------*/ @@ -9126,7 +9152,7 @@ static virDriver remote_driver = { remoteCPUBaseline, /* cpuBaseline */ remoteDomainGetJobInfo, /* domainGetJobInfo */ remoteDomainAbortJob, /* domainFinishJob */ - NULL, /* domainMigrateSetDowntime */ + remoteDomainMigrateSetDowntime, /* domainMigrateSetDowntime */ }; static virNetworkDriver network_driver = { -- 1.7.0.2
Signed-off-by: Jiri Denemark <jdenemar@redhat.com> --- daemon/remote.c | 29 ++++++++++ daemon/remote_dispatch_args.h | 1 + daemon/remote_dispatch_prototypes.h | 8 +++ daemon/remote_dispatch_table.h | 5 ++ src/remote/remote_protocol.c | 11 ++++ src/remote/remote_protocol.h | 97 +++++++++++++++++++---------------- src/remote/remote_protocol.x | 9 +++- 7 files changed, 115 insertions(+), 45 deletions(-) diff --git a/daemon/remote.c b/daemon/remote.c index 7c4339f..ae9a1be 100644 --- a/daemon/remote.c +++ b/daemon/remote.c @@ -5463,6 +5463,35 @@ remoteDispatchDomainAbortJob (struct qemud_server *server ATTRIBUTE_UNUSED, } +static int +remoteDispatchDomainMigrateSetDowntime(struct qemud_server *server ATTRIBUTE_UNUSED, + struct qemud_client *client ATTRIBUTE_UNUSED, + virConnectPtr conn, + remote_message_header *hdr ATTRIBUTE_UNUSED, + remote_error *rerr, + remote_domain_migrate_set_downtime_args *args, + void *ret ATTRIBUTE_UNUSED) +{ + virDomainPtr dom; + + dom = get_nonnull_domain(conn, args->dom); + if (dom == NULL) { + remoteDispatchConnError(rerr, conn); + return -1; + } + + if (virDomainMigrateSetDowntime(dom, args->downtime) == -1) { + virDomainFree(dom); + remoteDispatchConnError(rerr, conn); + return -1; + } + + virDomainFree(dom); + + return 0; +} + + /*----- Helpers. -----*/ /* get_nonnull_domain and get_nonnull_network turn an on-wire diff --git a/daemon/remote_dispatch_args.h b/daemon/remote_dispatch_args.h index f97155b..d771ad0 100644 --- a/daemon/remote_dispatch_args.h +++ b/daemon/remote_dispatch_args.h @@ -140,3 +140,4 @@ remote_cpu_baseline_args val_remote_cpu_baseline_args; remote_domain_get_job_info_args val_remote_domain_get_job_info_args; remote_domain_abort_job_args val_remote_domain_abort_job_args; + remote_domain_migrate_set_downtime_args val_remote_domain_migrate_set_downtime_args; diff --git a/daemon/remote_dispatch_prototypes.h b/daemon/remote_dispatch_prototypes.h index b81c8c3..c4cbe0b 100644 --- a/daemon/remote_dispatch_prototypes.h +++ b/daemon/remote_dispatch_prototypes.h @@ -378,6 +378,14 @@ static int remoteDispatchDomainMigratePrepareTunnel( remote_error *err, remote_domain_migrate_prepare_tunnel_args *args, void *ret); +static int remoteDispatchDomainMigrateSetDowntime( + struct qemud_server *server, + struct qemud_client *client, + virConnectPtr conn, + remote_message_header *hdr, + remote_error *err, + remote_domain_migrate_set_downtime_args *args, + void *ret); static int remoteDispatchDomainPinVcpu( struct qemud_server *server, struct qemud_client *client, diff --git a/daemon/remote_dispatch_table.h b/daemon/remote_dispatch_table.h index 5ad6bff..beb32cc 100644 --- a/daemon/remote_dispatch_table.h +++ b/daemon/remote_dispatch_table.h @@ -827,3 +827,8 @@ .args_filter = (xdrproc_t) xdr_remote_domain_abort_job_args, .ret_filter = (xdrproc_t) xdr_void, }, +{ /* DomainMigrateSetDowntime => 165 */ + .fn = (dispatch_fn) remoteDispatchDomainMigrateSetDowntime, + .args_filter = (xdrproc_t) xdr_remote_domain_migrate_set_downtime_args, + .ret_filter = (xdrproc_t) xdr_void, +}, diff --git a/src/remote/remote_protocol.c b/src/remote/remote_protocol.c index 701acab..6770cf0 100644 --- a/src/remote/remote_protocol.c +++ b/src/remote/remote_protocol.c @@ -3009,6 +3009,17 @@ xdr_remote_domain_abort_job_args (XDR *xdrs, remote_domain_abort_job_args *objp) } bool_t +xdr_remote_domain_migrate_set_downtime_args (XDR *xdrs, remote_domain_migrate_set_downtime_args *objp) +{ + + if (!xdr_remote_nonnull_domain (xdrs, &objp->dom)) + return FALSE; + if (!xdr_uint64_t (xdrs, &objp->downtime)) + return FALSE; + return TRUE; +} + +bool_t xdr_remote_procedure (XDR *xdrs, remote_procedure *objp) { diff --git a/src/remote/remote_protocol.h b/src/remote/remote_protocol.h index f76e6e5..8368572 100644 --- a/src/remote/remote_protocol.h +++ b/src/remote/remote_protocol.h @@ -4,51 +4,51 @@ */ #ifndef _RP_H_RPCGEN -# define _RP_H_RPCGEN +#define _RP_H_RPCGEN -# include <rpc/rpc.h> +#include <rpc/rpc.h> -# ifdef __cplusplus +#ifdef __cplusplus extern "C" { -# endif +#endif -# include "internal.h" -# include <arpa/inet.h> -# define REMOTE_MESSAGE_MAX 262144 -# define REMOTE_MESSAGE_HEADER_MAX 24 -# define REMOTE_MESSAGE_PAYLOAD_MAX 262120 -# define REMOTE_STRING_MAX 65536 +#include "internal.h" +#include <arpa/inet.h> +#define REMOTE_MESSAGE_MAX 262144 +#define REMOTE_MESSAGE_HEADER_MAX 24 +#define REMOTE_MESSAGE_PAYLOAD_MAX 262120 +#define REMOTE_STRING_MAX 65536 typedef char *remote_nonnull_string; typedef remote_nonnull_string *remote_string; -# define REMOTE_DOMAIN_ID_LIST_MAX 16384 -# define REMOTE_DOMAIN_NAME_LIST_MAX 1024 -# define REMOTE_CPUMAP_MAX 256 -# define REMOTE_VCPUINFO_MAX 2048 -# define REMOTE_CPUMAPS_MAX 16384 -# define REMOTE_MIGRATE_COOKIE_MAX 256 -# define REMOTE_NETWORK_NAME_LIST_MAX 256 -# define REMOTE_INTERFACE_NAME_LIST_MAX 256 -# define REMOTE_DEFINED_INTERFACE_NAME_LIST_MAX 256 -# define REMOTE_STORAGE_POOL_NAME_LIST_MAX 256 -# define REMOTE_STORAGE_VOL_NAME_LIST_MAX 1024 -# define REMOTE_NODE_DEVICE_NAME_LIST_MAX 16384 -# define REMOTE_NODE_DEVICE_CAPS_LIST_MAX 16384 -# define REMOTE_DOMAIN_SCHEDULER_PARAMETERS_MAX 16 -# define REMOTE_NODE_MAX_CELLS 1024 -# define REMOTE_AUTH_SASL_DATA_MAX 65536 -# define REMOTE_AUTH_TYPE_LIST_MAX 20 -# define REMOTE_DOMAIN_MEMORY_STATS_MAX 1024 -# define REMOTE_DOMAIN_BLOCK_PEEK_BUFFER_MAX 65536 -# define REMOTE_DOMAIN_MEMORY_PEEK_BUFFER_MAX 65536 -# define REMOTE_SECURITY_MODEL_MAX VIR_SECURITY_MODEL_BUFLEN -# define REMOTE_SECURITY_LABEL_MAX VIR_SECURITY_LABEL_BUFLEN -# define REMOTE_SECURITY_DOI_MAX VIR_SECURITY_DOI_BUFLEN -# define REMOTE_SECRET_VALUE_MAX 65536 -# define REMOTE_SECRET_UUID_LIST_MAX 16384 -# define REMOTE_CPU_BASELINE_MAX 256 +#define REMOTE_DOMAIN_ID_LIST_MAX 16384 +#define REMOTE_DOMAIN_NAME_LIST_MAX 1024 +#define REMOTE_CPUMAP_MAX 256 +#define REMOTE_VCPUINFO_MAX 2048 +#define REMOTE_CPUMAPS_MAX 16384 +#define REMOTE_MIGRATE_COOKIE_MAX 256 +#define REMOTE_NETWORK_NAME_LIST_MAX 256 +#define REMOTE_INTERFACE_NAME_LIST_MAX 256 +#define REMOTE_DEFINED_INTERFACE_NAME_LIST_MAX 256 +#define REMOTE_STORAGE_POOL_NAME_LIST_MAX 256 +#define REMOTE_STORAGE_VOL_NAME_LIST_MAX 1024 +#define REMOTE_NODE_DEVICE_NAME_LIST_MAX 16384 +#define REMOTE_NODE_DEVICE_CAPS_LIST_MAX 16384 +#define REMOTE_DOMAIN_SCHEDULER_PARAMETERS_MAX 16 +#define REMOTE_NODE_MAX_CELLS 1024 +#define REMOTE_AUTH_SASL_DATA_MAX 65536 +#define REMOTE_AUTH_TYPE_LIST_MAX 20 +#define REMOTE_DOMAIN_MEMORY_STATS_MAX 1024 +#define REMOTE_DOMAIN_BLOCK_PEEK_BUFFER_MAX 65536 +#define REMOTE_DOMAIN_MEMORY_PEEK_BUFFER_MAX 65536 +#define REMOTE_SECURITY_MODEL_MAX VIR_SECURITY_MODEL_BUFLEN +#define REMOTE_SECURITY_LABEL_MAX VIR_SECURITY_LABEL_BUFLEN +#define REMOTE_SECURITY_DOI_MAX VIR_SECURITY_DOI_BUFLEN +#define REMOTE_SECRET_VALUE_MAX 65536 +#define REMOTE_SECRET_UUID_LIST_MAX 16384 +#define REMOTE_CPU_BASELINE_MAX 256 typedef char remote_uuid[VIR_UUID_BUFLEN]; @@ -1704,8 +1704,14 @@ struct remote_domain_abort_job_args { remote_nonnull_domain dom; }; typedef struct remote_domain_abort_job_args remote_domain_abort_job_args; -# define REMOTE_PROGRAM 0x20008086 -# define REMOTE_PROTOCOL_VERSION 1 + +struct remote_domain_migrate_set_downtime_args { + remote_nonnull_domain dom; + uint64_t downtime; +}; +typedef struct remote_domain_migrate_set_downtime_args remote_domain_migrate_set_downtime_args; +#define REMOTE_PROGRAM 0x20008086 +#define REMOTE_PROTOCOL_VERSION 1 enum remote_procedure { REMOTE_PROC_OPEN = 1, @@ -1872,6 +1878,7 @@ enum remote_procedure { REMOTE_PROC_CPU_BASELINE = 162, REMOTE_PROC_DOMAIN_GET_JOB_INFO = 163, REMOTE_PROC_DOMAIN_ABORT_JOB = 164, + REMOTE_PROC_DOMAIN_MIGRATE_SET_DOWNTIME = 165, }; typedef enum remote_procedure remote_procedure; @@ -1889,7 +1896,7 @@ enum remote_message_status { REMOTE_CONTINUE = 2, }; typedef enum remote_message_status remote_message_status; -# define REMOTE_MESSAGE_HEADER_XDR_LEN 4 +#define REMOTE_MESSAGE_HEADER_XDR_LEN 4 struct remote_message_header { u_int prog; @@ -1903,7 +1910,7 @@ typedef struct remote_message_header remote_message_header; /* the xdr functions */ -# if defined(__STDC__) || defined(__cplusplus) +#if defined(__STDC__) || defined(__cplusplus) extern bool_t xdr_remote_nonnull_string (XDR *, remote_nonnull_string*); extern bool_t xdr_remote_string (XDR *, remote_string*); extern bool_t xdr_remote_uuid (XDR *, remote_uuid); @@ -2181,12 +2188,13 @@ extern bool_t xdr_remote_cpu_baseline_ret (XDR *, remote_cpu_baseline_ret*); extern bool_t xdr_remote_domain_get_job_info_args (XDR *, remote_domain_get_job_info_args*); extern bool_t xdr_remote_domain_get_job_info_ret (XDR *, remote_domain_get_job_info_ret*); extern bool_t xdr_remote_domain_abort_job_args (XDR *, remote_domain_abort_job_args*); +extern bool_t xdr_remote_domain_migrate_set_downtime_args (XDR *, remote_domain_migrate_set_downtime_args*); extern bool_t xdr_remote_procedure (XDR *, remote_procedure*); extern bool_t xdr_remote_message_type (XDR *, remote_message_type*); extern bool_t xdr_remote_message_status (XDR *, remote_message_status*); extern bool_t xdr_remote_message_header (XDR *, remote_message_header*); -# else /* K&R C */ +#else /* K&R C */ extern bool_t xdr_remote_nonnull_string (); extern bool_t xdr_remote_string (); extern bool_t xdr_remote_uuid (); @@ -2464,15 +2472,16 @@ extern bool_t xdr_remote_cpu_baseline_ret (); extern bool_t xdr_remote_domain_get_job_info_args (); extern bool_t xdr_remote_domain_get_job_info_ret (); extern bool_t xdr_remote_domain_abort_job_args (); +extern bool_t xdr_remote_domain_migrate_set_downtime_args (); extern bool_t xdr_remote_procedure (); extern bool_t xdr_remote_message_type (); extern bool_t xdr_remote_message_status (); extern bool_t xdr_remote_message_header (); -# endif /* K&R C */ +#endif /* K&R C */ -# ifdef __cplusplus +#ifdef __cplusplus } -# endif +#endif #endif /* !_RP_H_RPCGEN */ diff --git a/src/remote/remote_protocol.x b/src/remote/remote_protocol.x index 5e33da5..8654467 100644 --- a/src/remote/remote_protocol.x +++ b/src/remote/remote_protocol.x @@ -1517,6 +1517,12 @@ struct remote_domain_abort_job_args { }; +struct remote_domain_migrate_set_downtime_args { + remote_nonnull_domain dom; + unsigned hyper downtime; +}; + + /*----- Protocol. -----*/ /* Define the program number, protocol version and procedure numbers here. */ @@ -1703,7 +1709,8 @@ enum remote_procedure { REMOTE_PROC_DOMAIN_DETACH_DEVICE_FLAGS = 161, REMOTE_PROC_CPU_BASELINE = 162, REMOTE_PROC_DOMAIN_GET_JOB_INFO = 163, - REMOTE_PROC_DOMAIN_ABORT_JOB = 164 + REMOTE_PROC_DOMAIN_ABORT_JOB = 164, + REMOTE_PROC_DOMAIN_MIGRATE_SET_DOWNTIME = 165 /* * Notice how the entries are grouped in sets of 10 ? -- 1.7.0.2
Signed-off-by: Jiri Denemark <jdenemar@redhat.com> --- src/qemu/qemu_driver.c | 48 +++++++++++++++++++++++++++++++++++++++++- src/qemu/qemu_monitor.c | 15 +++++++++++++ src/qemu/qemu_monitor.h | 3 ++ src/qemu/qemu_monitor_json.c | 29 +++++++++++++++++++++++++ src/qemu/qemu_monitor_json.h | 3 ++ src/qemu/qemu_monitor_text.c | 27 +++++++++++++++++++++++ src/qemu/qemu_monitor_text.h | 3 ++ 7 files changed, 127 insertions(+), 1 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 8527c96..80973ea 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -9499,6 +9499,52 @@ cleanup: } +static int +qemuDomainMigrateSetDowntime(virDomainPtr dom, + unsigned long long downtime) +{ + struct qemud_driver *driver = dom->conn->privateData; + virDomainObjPtr vm; + qemuDomainObjPrivatePtr priv; + int ret = -1; + + qemuDriverLock(driver); + vm = virDomainFindByUUID(&driver->domains, dom->uuid); + qemuDriverUnlock(driver); + + if (!vm) { + char uuidstr[VIR_UUID_STRING_BUFLEN]; + virUUIDFormat(dom->uuid, uuidstr); + qemuReportError(VIR_ERR_NO_DOMAIN, + _("no domain with matching uuid '%s'"), uuidstr); + goto cleanup; + } + + if (qemuDomainObjBeginJob(vm) < 0) + goto cleanup; + + if (!virDomainObjIsActive(vm)) { + qemuReportError(VIR_ERR_OPERATION_INVALID, + "%s", _("domain is not running")); + goto endjob; + } + + priv = vm->privateData; + qemuDomainObjEnterMonitor(vm); + ret = qemuMonitorSetMigrationDowntime(priv->mon, downtime); + qemuDomainObjExitMonitor(vm); + +endjob: + if (qemuDomainObjEndJob(vm) == 0) + vm = NULL; + +cleanup: + if (vm) + virDomainObjUnlock(vm); + return ret; +} + + static virDriver qemuDriver = { VIR_DRV_QEMU, "QEMU", @@ -9580,7 +9626,7 @@ static virDriver qemuDriver = { qemuCPUBaseline, /* cpuBaseline */ qemuDomainGetJobInfo, /* domainGetJobInfo */ qemuDomainAbortJob, /* domainAbortJob */ - NULL, /* domainMigrateSetDowntime */ + qemuDomainMigrateSetDowntime, /* domainMigrateSetDowntime */ }; diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c index acc841b..6b68db8 100644 --- a/src/qemu/qemu_monitor.c +++ b/src/qemu/qemu_monitor.c @@ -1016,6 +1016,21 @@ int qemuMonitorSetMigrationSpeed(qemuMonitorPtr mon, return ret; } + +int qemuMonitorSetMigrationDowntime(qemuMonitorPtr mon, + unsigned long long downtime) +{ + int ret; + DEBUG("mon=%p, fd=%d downtime=%llu", mon, mon->fd, downtime); + + if (mon->json) + ret = qemuMonitorJSONSetMigrationDowntime(mon, downtime); + else + ret = qemuMonitorTextSetMigrationDowntime(mon, downtime); + return ret; +} + + int qemuMonitorGetMigrationStatus(qemuMonitorPtr mon, int *status, unsigned long long *transferred, diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h index cfb76b6..2557fb9 100644 --- a/src/qemu/qemu_monitor.h +++ b/src/qemu/qemu_monitor.h @@ -176,6 +176,9 @@ int qemuMonitorSavePhysicalMemory(qemuMonitorPtr mon, int qemuMonitorSetMigrationSpeed(qemuMonitorPtr mon, unsigned long bandwidth); +int qemuMonitorSetMigrationDowntime(qemuMonitorPtr mon, + unsigned long long downtime); + enum { QEMU_MONITOR_MIGRATION_STATUS_INACTIVE, QEMU_MONITOR_MIGRATION_STATUS_ACTIVE, diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index 7a263cb..b259452 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -1088,6 +1088,35 @@ int qemuMonitorJSONSetMigrationSpeed(qemuMonitorPtr mon, } +int qemuMonitorJSONSetMigrationDowntime(qemuMonitorPtr mon, + unsigned long long downtime) +{ + int ret; + char *downtimestr; + virJSONValuePtr cmd; + virJSONValuePtr reply = NULL; + if (virAsprintf(&downtimestr, "%llun", downtime) < 0) { + virReportOOMError(); + return -1; + } + cmd = qemuMonitorJSONMakeCommand("migrate_set_downtime", + "s:value", downtimestr, + NULL); + VIR_FREE(downtimestr); + if (!cmd) + return -1; + + ret = qemuMonitorJSONCommand(mon, cmd, &reply); + + if (ret == 0) + ret = qemuMonitorJSONCheckError(cmd, reply); + + virJSONValueFree(cmd); + virJSONValueFree(reply); + return ret; +} + + static int qemuMonitorJSONGetMigrationStatusReply(virJSONValuePtr reply, int *status, diff --git a/src/qemu/qemu_monitor_json.h b/src/qemu/qemu_monitor_json.h index 2906fee..fc05153 100644 --- a/src/qemu/qemu_monitor_json.h +++ b/src/qemu/qemu_monitor_json.h @@ -81,6 +81,9 @@ int qemuMonitorJSONSavePhysicalMemory(qemuMonitorPtr mon, int qemuMonitorJSONSetMigrationSpeed(qemuMonitorPtr mon, unsigned long bandwidth); +int qemuMonitorJSONSetMigrationDowntime(qemuMonitorPtr mon, + unsigned long long downtime); + int qemuMonitorJSONGetMigrationStatus(qemuMonitorPtr mon, int *status, unsigned long long *transferred, diff --git a/src/qemu/qemu_monitor_text.c b/src/qemu/qemu_monitor_text.c index b7c41a1..99f1180 100644 --- a/src/qemu/qemu_monitor_text.c +++ b/src/qemu/qemu_monitor_text.c @@ -999,6 +999,33 @@ cleanup: } +int qemuMonitorTextSetMigrationDowntime(qemuMonitorPtr mon, + unsigned long long downtime) +{ + char *cmd = NULL; + char *info = NULL; + int ret = -1; + + if (virAsprintf(&cmd, "migrate_set_downtime %llu", downtime) < 0) { + virReportOOMError(); + goto cleanup; + } + + if (qemuMonitorCommand(mon, cmd, &info) < 0) { + qemuReportError(VIR_ERR_OPERATION_FAILED, + "%s", _("could not set maximum migration downtime")); + goto cleanup; + } + + ret = 0; + +cleanup: + VIR_FREE(info); + VIR_FREE(cmd); + return ret; +} + + #define MIGRATION_PREFIX "Migration status: " #define MIGRATION_TRANSFER_PREFIX "transferred ram: " #define MIGRATION_REMAINING_PREFIX "remaining ram: " diff --git a/src/qemu/qemu_monitor_text.h b/src/qemu/qemu_monitor_text.h index 3215cae..4e1939c 100644 --- a/src/qemu/qemu_monitor_text.h +++ b/src/qemu/qemu_monitor_text.h @@ -83,6 +83,9 @@ int qemuMonitorTextSavePhysicalMemory(qemuMonitorPtr mon, int qemuMonitorTextSetMigrationSpeed(qemuMonitorPtr mon, unsigned long bandwidth); +int qemuMonitorTextSetMigrationDowntime(qemuMonitorPtr mon, + unsigned long long downtime); + int qemuMonitorTextGetMigrationStatus(qemuMonitorPtr mon, int *status, unsigned long long *transferred, -- 1.7.0.2
Actually there is a type in text monitor...
diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index 7a263cb..b259452 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -1088,6 +1088,35 @@ int qemuMonitorJSONSetMigrationSpeed(qemuMonitorPtr mon, }
+int qemuMonitorJSONSetMigrationDowntime(qemuMonitorPtr mon, + unsigned long long downtime) +{ + int ret; + char *downtimestr; + virJSONValuePtr cmd; + virJSONValuePtr reply = NULL; + if (virAsprintf(&downtimestr, "%llun", downtime) < 0) { + virReportOOMError(); + return -1; + } + cmd = qemuMonitorJSONMakeCommand("migrate_set_downtime", + "s:value", downtimestr, + NULL); ... diff --git a/src/qemu/qemu_monitor_text.c b/src/qemu/qemu_monitor_text.c index b7c41a1..99f1180 100644 --- a/src/qemu/qemu_monitor_text.c +++ b/src/qemu/qemu_monitor_text.c @@ -999,6 +999,33 @@ cleanup: }
+int qemuMonitorTextSetMigrationDowntime(qemuMonitorPtr mon, + unsigned long long downtime) +{ + char *cmd = NULL; + char *info = NULL; + int ret = -1; + + if (virAsprintf(&cmd, "migrate_set_downtime %llu", downtime) < 0) { + virReportOOMError(); + goto cleanup; + }
Here, we should tell qemu we're giving it time in nanoseconds just like we do in json monitor. I have a fixed version but I'll wait for changes possibly required by reviewers. Jirka
Signed-off-by: Jiri Denemark <jdenemar@redhat.com> --- I forgot to include virsh changes in the series introducing virDomainMigrateSetDowntime API call, so here it is... tools/virsh.c | 62 ++++++++++++++++++++++++++++++++++++++++++++++++++++++-- 1 files changed, 59 insertions(+), 3 deletions(-) diff --git a/tools/virsh.c b/tools/virsh.c index aa85ee6..f004ac0 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -125,6 +125,7 @@ typedef enum { VSH_OT_BOOL, /* boolean option */ VSH_OT_STRING, /* string option */ VSH_OT_INT, /* int option */ + VSH_OT_FLOAT, /* decimal option */ VSH_OT_DATA /* string data (as non-option) */ } vshCmdOptType; @@ -227,6 +228,8 @@ static vshCmdOpt *vshCommandOpt(const vshCmd *cmd, const char *name); static int vshCommandOptInt(const vshCmd *cmd, const char *name, int *found); static char *vshCommandOptString(const vshCmd *cmd, const char *name, int *found); +static double vshCommandOptFloat(const vshCmd *cmd, const char *name, + int *found); #if 0 static int vshCommandOptStringList(const vshCmd *cmd, const char *name, char ***data); #endif @@ -2747,6 +2750,7 @@ static const vshCmdOptDef opts_migrate[] = { {"persistent", VSH_OT_BOOL, 0, N_("persist VM on destination")}, {"undefinesource", VSH_OT_BOOL, 0, N_("undefine VM on source")}, {"suspend", VSH_OT_BOOL, 0, N_("do not restart the domain on the destination host")}, + {"downtime", VSH_OT_FLOAT, 0, N_("maximum tolerable downtime (in seconds) for migration")}, {"domain", VSH_OT_DATA, VSH_OFLAG_REQ, N_("domain name, id or uuid")}, {"desturi", VSH_OT_DATA, VSH_OFLAG_REQ, N_("connection URI of the destination host")}, {"migrateuri", VSH_OT_DATA, 0, N_("migration URI, usually can be omitted")}, @@ -2762,6 +2766,7 @@ cmdMigrate (vshControl *ctl, const vshCmd *cmd) const char *migrateuri; const char *dname; int flags = 0, found, ret = FALSE; + double downtime; if (!vshConnectionUsability (ctl, ctl->conn, TRUE)) return FALSE; @@ -2794,6 +2799,19 @@ cmdMigrate (vshControl *ctl, const vshCmd *cmd) if (vshCommandOptBool (cmd, "suspend")) flags |= VIR_MIGRATE_PAUSED; + downtime = vshCommandOptFloat(cmd, "downtime", &found); + if (found) { + unsigned long long nanoseconds = downtime * 1e9; + + if (nanoseconds <= 0) { + vshError(ctl, "%s", _("migrate: Invalid downtime")); + goto done; + } + + if (virDomainMigrateSetDowntime(dom, nanoseconds)) + goto done; + } + if ((flags & VIR_MIGRATE_PEER2PEER) || vshCommandOptBool (cmd, "direct")) { /* For peer2peer migration or direct migration we only expect one URI @@ -7938,6 +7956,9 @@ vshCmddefHelp(vshControl *ctl, const char *cmdname) else if (opt->type == VSH_OT_INT) /* xgettext:c-format */ fmt = _("[--%s <number>]"); + else if (opt->type == VSH_OT_FLOAT) + /* xgettext:c-format */ + fmt = _("[--%s <decimal>]"); else if (opt->type == VSH_OT_STRING) /* xgettext:c-format */ fmt = _("[--%s <string>]"); @@ -7965,6 +7986,8 @@ vshCmddefHelp(vshControl *ctl, const char *cmdname) snprintf(buf, sizeof(buf), "--%s", opt->name); else if (opt->type == VSH_OT_INT) snprintf(buf, sizeof(buf), _("--%s <number>"), opt->name); + else if (opt->type == VSH_OT_FLOAT) + snprintf(buf, sizeof(buf), _("--%s <decimal>"), opt->name); else if (opt->type == VSH_OT_STRING) snprintf(buf, sizeof(buf), _("--%s <string>"), opt->name); else if (opt->type == VSH_OT_DATA) @@ -8065,6 +8088,30 @@ vshCommandOptString(const vshCmd *cmd, const char *name, int *found) return arg && arg->data && *arg->data ? arg->data : NULL; } +/* + * Returns option as DOUBLE + */ +static double +vshCommandOptFloat(const vshCmd *cmd, const char *name, int *found) +{ + vshCmdOpt *arg = vshCommandOpt(cmd, name); + int num_found = FALSE; + double res = 0; + char *end_p = NULL; + + if ((arg != NULL) && (arg->data != NULL)) { + errno = 0; + res = strtod(arg->data, &end_p); + if ((arg->data == end_p) || (*end_p != 0) || errno) + num_found = FALSE; + else + num_found = TRUE; + } + if (found) + *found = num_found; + return res; +} + #if 0 static int vshCommandOptStringList(const vshCmd *cmd, const char *name, char ***data) @@ -8576,11 +8623,20 @@ vshCommandParse(vshControl *ctl, char *cmdstr) if (tk == VSH_TK_ERROR) goto syntaxError; if (tk != VSH_TK_DATA) { + const char *type; + switch (opt->type) { + case VSH_OT_INT: + type = _("number"); + break; + case VSH_OT_FLOAT: + type = _("decimal"); + break; + default: + type = _("string"); + } vshError(ctl, _("expected syntax: --%s <%s>"), - opt->name, - opt->type == - VSH_OT_INT ? _("number") : _("string")); + opt->name, type); goto syntaxError; } } -- 1.7.0.2
2010/3/16 Jiri Denemark <jdenemar@redhat.com>:
Signed-off-by: Jiri Denemark <jdenemar@redhat.com> ---
@@ -2794,6 +2799,19 @@ cmdMigrate (vshControl *ctl, const vshCmd *cmd) if (vshCommandOptBool (cmd, "suspend")) flags |= VIR_MIGRATE_PAUSED;
+ downtime = vshCommandOptFloat(cmd, "downtime", &found); + if (found) { + unsigned long long nanoseconds = downtime * 1e9; + + if (nanoseconds <= 0) { + vshError(ctl, "%s", _("migrate: Invalid downtime")); + goto done; + } + + if (virDomainMigrateSetDowntime(dom, nanoseconds))
Is this persistent, or only valid for the next migration? For example, I do a migration with --downtime and afterwards I do another migration with the same domain, but this time I don't specify a downtime. Would the first downtime still apply to the second migration? Matthias
@@ -2794,6 +2799,19 @@ cmdMigrate (vshControl *ctl, const vshCmd *cmd) if (vshCommandOptBool (cmd, "suspend")) flags |= VIR_MIGRATE_PAUSED;
+ downtime = vshCommandOptFloat(cmd, "downtime", &found); + if (found) { + unsigned long long nanoseconds = downtime * 1e9; + + if (nanoseconds <= 0) { + vshError(ctl, "%s", _("migrate: Invalid downtime")); + goto done; + } + + if (virDomainMigrateSetDowntime(dom, nanoseconds))
Is this persistent, or only valid for the next migration? For example, I do a migration with --downtime and afterwards I do another migration with the same domain, but this time I don't specify a downtime. Would the first downtime still apply to the second migration?
Actually, this is a very good question since it reveals the API wasn't designed well enough. Current implementation doesn't do much with the value passed to virDomainMigrateSetDowntime, it just sends it to the appropriate hypervisor (if it supports such functionality). In other words, the behavior may differ for different hypervisors. Which doesn't seem to be a good thing for a libvirt's public API. So we should decide whether the effect of calling this API should be one-time or persistent and emulate that behavior for hypervisors which don't support it. We also need flags parameter so that we can change that behavior in the future. Opinions? Jirka
2010/3/17 Jiri Denemark <jdenemar@redhat.com>:
@@ -2794,6 +2799,19 @@ cmdMigrate (vshControl *ctl, const vshCmd *cmd) if (vshCommandOptBool (cmd, "suspend")) flags |= VIR_MIGRATE_PAUSED;
+ downtime = vshCommandOptFloat(cmd, "downtime", &found); + if (found) { + unsigned long long nanoseconds = downtime * 1e9; + + if (nanoseconds <= 0) { + vshError(ctl, "%s", _("migrate: Invalid downtime")); + goto done; + } + + if (virDomainMigrateSetDowntime(dom, nanoseconds))
Is this persistent, or only valid for the next migration? For example, I do a migration with --downtime and afterwards I do another migration with the same domain, but this time I don't specify a downtime. Would the first downtime still apply to the second migration?
Actually, this is a very good question since it reveals the API wasn't designed well enough. Current implementation doesn't do much with the value passed to virDomainMigrateSetDowntime, it just sends it to the appropriate hypervisor (if it supports such functionality). In other words, the behavior may differ for different hypervisors. Which doesn't seem to be a good thing for a libvirt's public API. So we should decide whether the effect of calling this API should be one-time or persistent and emulate that behavior for hypervisors which don't support it. We also need flags parameter so that we can change that behavior in the future.
Opinions?
Jirka
Your patch series haven't been reviewed in detail yet, so some general comments about it. First of all, as with all new public API functions, virDomainMigrateSetDowntime should have an 'unsigned int flags' parameter. Even if it's currently unused it could come in handy in the future. Maybe the function should be called virDomainMigrateSetTolerableDowntime or virDomainMigrateSetMaxDowntime, to emphasis that this downtime is an upper limit. There is no getter for the max downtime value, I think there should be one. Regarding the persistence, if the max downtime is implemented in the way of a setter/(getter), it should be persistent. For a per-migration option it should be a parameter to the migrate function like bandwidth, but we can change the migrate function anymore. Therefore, the max downtime should be a persistent option per domain, like the autostart flag. Matthias
On Wed, Mar 17, 2010 at 11:05:35AM +0100, Matthias Bolte wrote:
2010/3/17 Jiri Denemark <jdenemar@redhat.com>:
@@ -2794,6 +2799,19 @@ cmdMigrate (vshControl *ctl, const vshCmd *cmd) if (vshCommandOptBool (cmd, "suspend")) flags |= VIR_MIGRATE_PAUSED;
+ downtime = vshCommandOptFloat(cmd, "downtime", &found); + if (found) { + unsigned long long nanoseconds = downtime * 1e9; + + if (nanoseconds <= 0) { + vshError(ctl, "%s", _("migrate: Invalid downtime")); + goto done; + } + + if (virDomainMigrateSetDowntime(dom, nanoseconds))
Is this persistent, or only valid for the next migration? For example, I do a migration with --downtime and afterwards I do another migration with the same domain, but this time I don't specify a downtime. Would the first downtime still apply to the second migration?
Actually, this is a very good question since it reveals the API wasn't designed well enough. Current implementation doesn't do much with the value passed to virDomainMigrateSetDowntime, it just sends it to the appropriate hypervisor (if it supports such functionality). In other words, the behavior may differ for different hypervisors. Which doesn't seem to be a good thing for a libvirt's public API. So we should decide whether the effect of calling this API should be one-time or persistent and emulate that behavior for hypervisors which don't support it. We also need flags parameter so that we can change that behavior in the future.
Opinions?
Jirka
Your patch series haven't been reviewed in detail yet, so some general comments about it.
First of all, as with all new public API functions, virDomainMigrateSetDowntime should have an 'unsigned int flags' parameter. Even if it's currently unused it could come in handy in the future.
Maybe the function should be called virDomainMigrateSetTolerableDowntime or virDomainMigrateSetMaxDowntime, to emphasis that this downtime is an upper limit.
There is no getter for the max downtime value, I think there should be one.
Regarding the persistence, if the max downtime is implemented in the way of a setter/(getter), it should be persistent. For a per-migration option it should be a parameter to the migrate function like bandwidth, but we can change the migrate function anymore. Therefore, the max downtime should be a persistent option per domain, like the autostart flag.
Conceptually this doesn't make sense. The max downtime parameter is a live tunable that a management app will set based on the conditions at the time of migration. Suitable value is dependant on the workload of the guest (how quickly it is dirtying RAM) and network bandwidth between hosts (how quickly RAM is transferred). You would only increase the max downtime if the VM was not able to complete migration promptly enough. So it shouldn't be persistent, and I don't think there's any compelling need for a getter either. Regards, Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://deltacloud.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
2010/3/17 Daniel P. Berrange <berrange@redhat.com>:
On Wed, Mar 17, 2010 at 11:05:35AM +0100, Matthias Bolte wrote:
2010/3/17 Jiri Denemark <jdenemar@redhat.com>:
@@ -2794,6 +2799,19 @@ cmdMigrate (vshControl *ctl, const vshCmd *cmd) if (vshCommandOptBool (cmd, "suspend")) flags |= VIR_MIGRATE_PAUSED;
+ downtime = vshCommandOptFloat(cmd, "downtime", &found); + if (found) { + unsigned long long nanoseconds = downtime * 1e9; + + if (nanoseconds <= 0) { + vshError(ctl, "%s", _("migrate: Invalid downtime")); + goto done; + } + + if (virDomainMigrateSetDowntime(dom, nanoseconds))
Is this persistent, or only valid for the next migration? For example, I do a migration with --downtime and afterwards I do another migration with the same domain, but this time I don't specify a downtime. Would the first downtime still apply to the second migration?
Actually, this is a very good question since it reveals the API wasn't designed well enough. Current implementation doesn't do much with the value passed to virDomainMigrateSetDowntime, it just sends it to the appropriate hypervisor (if it supports such functionality). In other words, the behavior may differ for different hypervisors. Which doesn't seem to be a good thing for a libvirt's public API. So we should decide whether the effect of calling this API should be one-time or persistent and emulate that behavior for hypervisors which don't support it. We also need flags parameter so that we can change that behavior in the future.
Opinions?
Jirka
Your patch series haven't been reviewed in detail yet, so some general comments about it.
First of all, as with all new public API functions, virDomainMigrateSetDowntime should have an 'unsigned int flags' parameter. Even if it's currently unused it could come in handy in the future.
Maybe the function should be called virDomainMigrateSetTolerableDowntime or virDomainMigrateSetMaxDowntime, to emphasis that this downtime is an upper limit.
There is no getter for the max downtime value, I think there should be one.
Regarding the persistence, if the max downtime is implemented in the way of a setter/(getter), it should be persistent. For a per-migration option it should be a parameter to the migrate function like bandwidth, but we can change the migrate function anymore. Therefore, the max downtime should be a persistent option per domain, like the autostart flag.
Conceptually this doesn't make sense. The max downtime parameter is a live tunable that a management app will set based on the conditions at the time of migration. Suitable value is dependant on the workload of the guest (how quickly it is dirtying RAM) and network bandwidth between hosts (how quickly RAM is transferred). You would only increase the max downtime if the VM was not able to complete migration promptly enough. So it shouldn't be persistent, and I don't think there's any compelling need for a getter either.
Regards, Daniel
True. I just had the API POV in mind and forgot about the rest. So we add a new function to configure a per-migration option because we cannot change the existing migrate functions to add a new parameter. Matthias
On Wed, Mar 17, 2010 at 12:02:27PM +0100, Matthias Bolte wrote:
2010/3/17 Daniel P. Berrange <berrange@redhat.com>:
On Wed, Mar 17, 2010 at 11:05:35AM +0100, Matthias Bolte wrote:
Conceptually this doesn't make sense. The max downtime parameter is a live tunable that a management app will set based on the conditions at the time of migration. Suitable value is dependant on the workload of the guest (how quickly it is dirtying RAM) and network bandwidth between hosts (how quickly RAM is transferred). You would only increase the max downtime if the VM was not able to complete migration promptly enough. So it shouldn't be persistent, and I don't think there's any compelling need for a getter either.
Regards, Daniel
True. I just had the API POV in mind and forgot about the rest.
So we add a new function to configure a per-migration option because we cannot change the existing migrate functions to add a new parameter.
It isn't sufficient to set this in virDomainMigrate, since it needs to be tuned (possibly multiple times) while migration is taking place. Regards, Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://deltacloud.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
2010/3/17 Daniel P. Berrange <berrange@redhat.com>:
On Wed, Mar 17, 2010 at 12:02:27PM +0100, Matthias Bolte wrote:
2010/3/17 Daniel P. Berrange <berrange@redhat.com>:
On Wed, Mar 17, 2010 at 11:05:35AM +0100, Matthias Bolte wrote:
Conceptually this doesn't make sense. The max downtime parameter is a live tunable that a management app will set based on the conditions at the time of migration. Suitable value is dependant on the workload of the guest (how quickly it is dirtying RAM) and network bandwidth between hosts (how quickly RAM is transferred). You would only increase the max downtime if the VM was not able to complete migration promptly enough. So it shouldn't be persistent, and I don't think there's any compelling need for a getter either.
Regards, Daniel
True. I just had the API POV in mind and forgot about the rest.
So we add a new function to configure a per-migration option because we cannot change the existing migrate functions to add a new parameter.
It isn't sufficient to set this in virDomainMigrate, since it needs to be tuned (possibly multiple times) while migration is taking place.
Regards, Daniel
Ah, now I understand how this is supposed to be used and how to it should work! With that concept in mind the original patch is fine. Matthias
On 03/16/2010 08:56 AM, Jiri Denemark wrote:
@@ -2794,6 +2799,19 @@ cmdMigrate (vshControl *ctl, const vshCmd *cmd) if (vshCommandOptBool (cmd, "suspend")) flags |= VIR_MIGRATE_PAUSED;
+ downtime = vshCommandOptFloat(cmd, "downtime", &found); + if (found) { + unsigned long long nanoseconds = downtime * 1e9; + + if (nanoseconds <= 0) { + vshError(ctl, "%s", _("migrate: Invalid downtime")); + goto done; + }
You are only detecting negative time. But what about overflow, or if downtime was NaN or inf?
+ else if (opt->type == VSH_OT_FLOAT) + /* xgettext:c-format */ + fmt = _("[--%s <decimal>]");
<decimal> reminds me of base-10 integers, not floating point. It looks like this is the first time we are accepting floating point; should we use <float> or <floating-point> instead as the terminology?
+/* + * Returns option as DOUBLE + */ +static double +vshCommandOptFloat(const vshCmd *cmd, const char *name, int *found) +{ + vshCmdOpt *arg = vshCommandOpt(cmd, name); + int num_found = FALSE; + double res = 0; + char *end_p = NULL; + + if ((arg != NULL) && (arg->data != NULL)) { + errno = 0; + res = strtod(arg->data, &end_p);
Should we be using the gnulib strtod module here? -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
@@ -2794,6 +2799,19 @@ cmdMigrate (vshControl *ctl, const vshCmd *cmd) if (vshCommandOptBool (cmd, "suspend")) flags |= VIR_MIGRATE_PAUSED;
+ downtime = vshCommandOptFloat(cmd, "downtime", &found); + if (found) { + unsigned long long nanoseconds = downtime * 1e9; + + if (nanoseconds <= 0) { + vshError(ctl, "%s", _("migrate: Invalid downtime")); + goto done; + }
You are only detecting negative time. But what about overflow, or if downtime was NaN or inf?
Yeah, the test is completely wrong. It's effectively detecting only 0 time, unsigned cannot really be negative ;)
+ else if (opt->type == VSH_OT_FLOAT) + /* xgettext:c-format */ + fmt = _("[--%s <decimal>]");
<decimal> reminds me of base-10 integers, not floating point. It looks like this is the first time we are accepting floating point; should we use <float> or <floating-point> instead as the terminology?
That's probably better but this part will be removed completely in the new version so we don't have to worry about the terminology.
+ res = strtod(arg->data, &end_p);
Should we be using the gnulib strtod module here?
Why? It doesn't seem to be any better than C89 strtod. Or did I miss anything? Jirka
On 03/18/2010 05:17 AM, Jiri Denemark wrote:
+ res = strtod(arg->data, &end_p);
Should we be using the gnulib strtod module here?
Why? It doesn't seem to be any better than C89 strtod. Or did I miss anything?
strtod is broken on a number of platforms in various ways; most of them related to parsing the new formats required by C99, but there are some other bugs even with C89 parsing as well: http://git.sv.gnu.org/cgit/gnulib.git/tree/doc/posix-functions/strtod.texi -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
On Thu, Mar 18, 2010 at 08:35:18AM -0600, Eric Blake wrote:
On 03/18/2010 05:17 AM, Jiri Denemark wrote:
+ res = strtod(arg->data, &end_p);
Should we be using the gnulib strtod module here?
Why? It doesn't seem to be any better than C89 strtod. Or did I miss anything?
strtod is broken on a number of platforms in various ways; most of them related to parsing the new formats required by C99, but there are some other bugs even with C89 parsing as well:
http://git.sv.gnu.org/cgit/gnulib.git/tree/doc/posix-functions/strtod.texi
Actually, virsh should be using virStrToDouble(). If there are issues with strtod(),then virStrToDouble is the place to fix them, using gnulib for it if applicable Regards Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://deltacloud.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
participants (4)
-
Daniel P. Berrange -
Eric Blake -
Jiri Denemark -
Matthias Bolte