On 07/28/2017 10:57 AM, Scott Garfinkle wrote:

From: Scott Garfinkle <seg@us.ibm.com>

Since you'll be updating anyway from your regular email (e.g. non lotus notes account)... Add a brief commit message here. It can be as simple as introduce the xxx API in order to allow fetching the current downtime value.

--- include/libvirt/libvirt-domain.h | 4 ++++ src/driver-hypervisor.h | 6 ++++++ src/libvirt-domain.c | 45 ++++++++++++++++++++++++++++++++++++++++ src/libvirt_public.syms | 4 ++++ 4 files changed, 59 insertions(+)

diff --git a/include/libvirt/libvirt-domain.h b/include/libvirt/libvirt-domain.h index a3bb9cb..22618b0 100644 --- a/include/libvirt/libvirt-domain.h +++ b/include/libvirt/libvirt-domain.h @@ -1043,6 +1043,10 @@ int virDomainMigrateSetMaxDowntime (virDomainPtr domain, unsigned long long downtime, unsigned int flags);

+int virDomainMigrateGetMaxDowntime (virDomainPtr domain, ^ + unsigned long long *downtime, + unsigned int flags); +

I know you're just copying the Set function, but no space between e and ( - will affect the subsequent 3 lines as well. May as well get the "new" ones right. If someone (or you) wants to fix the existing ones in (a) separate patch(es), then more have at it!

int virDomainMigrateGetCompressionCache(virDomainPtr domain, unsigned long long *cacheSize, unsigned int flags); diff --git a/src/driver-hypervisor.h b/src/driver-hypervisor.h index 3053d7a..7d32cad 100644 --- a/src/driver-hypervisor.h +++ b/src/driver-hypervisor.h @@ -702,6 +702,11 @@ typedef int unsigned int flags);

typedef int +(*virDrvDomainMigrateGetMaxDowntime)(virDomainPtr domain, + unsigned long long *downtime, + unsigned int flags); + +typedef int (*virDrvDomainMigrateGetCompressionCache)(virDomainPtr domain, unsigned long long *cacheSize, unsigned int flags); @@ -1412,6 +1417,7 @@ struct _virHypervisorDriver { virDrvDomainGetJobInfo domainGetJobInfo; virDrvDomainGetJobStats domainGetJobStats; virDrvDomainAbortJob domainAbortJob; + virDrvDomainMigrateGetMaxDowntime domainMigrateGetMaxDowntime; virDrvDomainMigrateSetMaxDowntime domainMigrateSetMaxDowntime; virDrvDomainMigrateGetCompressionCache domainMigrateGetCompressionCache; virDrvDomainMigrateSetCompressionCache domainMigrateSetCompressionCache; diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c index 4033ae8..b78ea1c 100644 --- a/src/libvirt-domain.c +++ b/src/libvirt-domain.c @@ -8781,6 +8781,51 @@ virDomainMigrateSetMaxDowntime(virDomainPtr domain,

/** + * virDomainMigrateGetMaxDowntime: + * @domain: a domain object + * @downtime: return value of the maximum tolerable downtime for live + * migration, in milliseconds + * @flags: extra flags; not used yet, so callers should always pass 0 + * + * Gets current maximum tolerable time for which the domain may be paused + * at the end of live migration. It's supposed to be called while the domain is + * being live-migrated as a reaction to migration progress.

s/It's supposed ... migration progress.// IOW: No need to cut-n-paste the 'Set' text regarding needing to be done during the migration... You could add something else, but I don't think it's necessary.

+ * + * Returns 0 in case of success, -1 otherwise. + */ +int +virDomainMigrateGetMaxDowntime(virDomainPtr domain, + unsigned long long *downtime, + unsigned int flags) +{ + virConnectPtr conn; + + VIR_DOMAIN_DEBUG(domain, "downtime = %p, flags=%x", downtime, flags); + + virResetLastError(); + + virCheckDomainReturn(domain, -1); + conn = domain->conn; + + virCheckNonNullArgGoto(downtime, error); + + /* unlike SetMaxDowntime, it's okay for the connection to be readonly */

No need for the comment...

+ + if (conn->driver->domainMigrateGetMaxDowntime) { + if (conn->driver->domainMigrateGetMaxDowntime(domain, + downtime, flags) < 0)

Personally I would have put downtime on the previous line...

+ goto error; + return 0; + } + + virReportUnsupportedError(); + error: + virDispatchError(conn); + return -1; +} + + +/** * virDomainMigrateGetCompressionCache: * @domain: a domain object * @cacheSize: return value of current size of the cache (in bytes) diff --git a/src/libvirt_public.syms b/src/libvirt_public.syms index fac77fb..da5692a 100644 --- a/src/libvirt_public.syms +++ b/src/libvirt_public.syms @@ -768,4 +768,8 @@ LIBVIRT_3.4.0 { virStreamSparseSendAll; } LIBVIRT_3.1.0;

+LIBVIRT_3.6.0 {

This will be 3.7.0 at the earliest now. John

+ global: + virDomainMigrateGetMaxDowntime; +} LIBVIRT_3.4.0; # .... define new API here using predicted next version number ....

On 07/28/2017 10:57 AM, Scott Garfinkle wrote:

From: Scott Garfinkle <seg@us.ibm.com>

Need a commit message. Typically this is something like "Set up wire and protocol for xxx" (see commit id 20ffaf59d for the SetMaxDowntime example)

--- src/qemu/qemu_driver.c | 51 ++++++++++++++++++++++++++++++++++++++++++++ src/qemu/qemu_monitor.h | 3 +++ src/qemu/qemu_monitor_json.c | 4 ++++ src/remote/remote_driver.c | 1 + src/remote/remote_protocol.x | 16 +++++++++++++- src/remote_protocol-structs | 8 +++++++ 6 files changed, 82 insertions(+), 1 deletion(-)

This patch failed 'make check syntax-check' (see below)

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index f7b4cc3..8b8ae57 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -13147,6 +13147,56 @@ qemuDomainMigrateSetMaxDowntime(virDomainPtr dom, return ret; }

+ +static int +qemuDomainMigrateGetMaxDowntime(virDomainPtr dom, + unsigned long long *downtime, + unsigned int flags) +{ + virQEMUDriverPtr driver = dom->conn->privateData; + virDomainObjPtr vm; + qemuDomainObjPrivatePtr priv; + qemuMonitorMigrationParams migparams;

s/migparams;/migparams = { 0 }; just to be clear - so that downtimeLimit_set isn't 1 due to random stack stuff

+ int ret = -1; + + virCheckFlags(0, -1); + + if (!(vm = qemuDomObjFromDomain(dom))) + goto cleanup;

Could return -1 here

+ + if (virDomainMigrateGetMaxDowntimeEnsureACL(dom->conn, vm->def) < 0) + goto cleanup; + + if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_QUERY) < 0) + goto cleanup; + + if (!virDomainObjIsActive(vm)) { + virReportError(VIR_ERR_OPERATION_INVALID, + "%s", _("domain is not running")); + goto endjob; + } + + priv = vm->privateData; + qemuDomainObjEnterMonitor(driver, vm); + + if (!(ret = qemuMonitorGetMigrationParams(priv->mon, &migparams))) {

I go back and forth on this "if (!(ret = ())) vs. "if ((ret = ()) == 0)" - I like the latter mainly because when I see (!( I'm thinking pointers.

+ if (migparams.downtimeLimit_set) + *downtime = migparams.downtimeLimit; + else + ret = -1;

I think this needs some sort of error message; otherwise, if the "downtime-limit" doesn't exist from a "query-migrate-parameters" then you'd get the fallback libvirt failed for some unknown reason error message. Other places that return -1 here would all elicit some message... Thus: if (qemuMonitorGetMigrationParams(priv->mon, &migparams) == 0) { if (migparams.downtimeLimit_set) { *downtime = migparams.downtimeLimit; ret = 0; } else { virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s", _("Getting maximum downtime limit is not supported")); } } (or some error message such as that - I'm not sure if there is a way to determine like the cache code does that the parameter exists.

+ } + + if (qemuDomainObjExitMonitor(driver, vm) < 0) + ret = -1; + + endjob: + qemuDomainObjEndJob(driver, vm); + + cleanup: + virDomainObjEndAPI(&vm); + return ret; +} +

Extra space after too...

static int qemuDomainMigrateGetCompressionCache(virDomainPtr dom, unsigned long long *cacheSize, @@ -20826,6 +20876,7 @@ static virHypervisorDriver qemuHypervisorDriver = { .domainGetJobInfo = qemuDomainGetJobInfo, /* 0.7.7 */ .domainGetJobStats = qemuDomainGetJobStats, /* 1.0.3 */ .domainAbortJob = qemuDomainAbortJob, /* 0.7.7 */ + .domainMigrateGetMaxDowntime = qemuDomainMigrateGetMaxDowntime, /* 3.6.0 */

3.7.0

.domainMigrateSetMaxDowntime = qemuDomainMigrateSetMaxDowntime, /* 0.8.0 */ .domainMigrateGetCompressionCache = qemuDomainMigrateGetCompressionCache, /* 1.0.3 */ .domainMigrateSetCompressionCache = qemuDomainMigrateSetCompressionCache, /* 1.0.3 */ diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h index 31f7e97..9805a33 100644 --- a/src/qemu/qemu_monitor.h +++ b/src/qemu/qemu_monitor.h @@ -627,6 +627,9 @@ struct _qemuMonitorMigrationParams { * whereas, some string value indicates we can support setting/clearing */ char *migrateTLSAlias; char *migrateTLSHostname; + + bool downtimeLimit_set; + unsigned long long downtimeLimit; };

int qemuMonitorGetMigrationParams(qemuMonitorPtr mon, diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index b8a6815..b7b809d 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -2703,6 +2703,10 @@ qemuMonitorJSONGetMigrationParams(qemuMonitorPtr mon, PARSE(cpuThrottleInitial, "cpu-throttle-initial"); PARSE(cpuThrottleIncrement, "cpu-throttle-increment");

+ if (virJSONValueObjectGetNumberUlong(result, "downtime-limit", + ¶ms->downtimeLimit) == 0) + params->downtimeLimit_set = true; + #undef PARSE

Put the new code after the #undef since it's not part of the PARSE

if ((tlsStr = virJSONValueObjectGetString(result, "tls-creds"))) { diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c index a57d25f..aa8d8a1 100644 --- a/src/remote/remote_driver.c +++ b/src/remote/remote_driver.c @@ -8400,6 +8400,7 @@ static virHypervisorDriver hypervisor_driver = { .domainGetJobInfo = remoteDomainGetJobInfo, /* 0.7.7 */ .domainGetJobStats = remoteDomainGetJobStats, /* 1.0.3 */ .domainAbortJob = remoteDomainAbortJob, /* 0.7.7 */ + .domainMigrateGetMaxDowntime = remoteDomainMigrateGetMaxDowntime, /* 3.6.0 */

3.7.0

.domainMigrateSetMaxDowntime = remoteDomainMigrateSetMaxDowntime, /* 0.8.0 */ .domainMigrateGetCompressionCache = remoteDomainMigrateGetCompressionCache, /* 1.0.3 */ .domainMigrateSetCompressionCache = remoteDomainMigrateSetCompressionCache, /* 1.0.3 */ diff --git a/src/remote/remote_protocol.x b/src/remote/remote_protocol.x index aa0aa38..e1f4e27 100644 --- a/src/remote/remote_protocol.x +++ b/src/remote/remote_protocol.x @@ -2326,6 +2326,15 @@ struct remote_domain_abort_job_args { };

+struct remote_domain_migrate_get_max_downtime_args { + remote_nonnull_domain dom; + unsigned int flags; +}; + +struct remote_domain_migrate_get_max_downtime_ret { + unsigned hyper downtime; /* insert@1 */ +}; + struct remote_domain_migrate_set_max_downtime_args { remote_nonnull_domain dom; unsigned hyper downtime; @@ -6064,7 +6073,12 @@ enum remote_procedure { * @generate: both * @acl: domain:write */ - REMOTE_PROC_DOMAIN_SET_BLOCK_THRESHOLD = 386 + REMOTE_PROC_DOMAIN_SET_BLOCK_THRESHOLD = 386,

+ /** + * @generate: both + * @acl: domain:migrate + */ + REMOTE_PROC_DOMAIN_MIGRATE_GET_MAX_DOWNTIME = 387

}; diff --git a/src/remote_protocol-structs b/src/remote_protocol-structs index a46fe37..5804e60 100644 --- a/src/remote_protocol-structs +++ b/src/remote_protocol-structs @@ -1778,6 +1778,13 @@ struct remote_domain_migrate_set_max_downtime_args { uint64_t downtime; u_int flags; }; +struct remote_domain_migrate_get_max_downtime_args { + remote_nonnull_domain dom; + u_int flags; +}; +struct remote_domain_migrate_get_max_downtime_ret { + uint64_t downtime; +};

These two should be above the "set_max_downtime_args" (check/syntax-check failure)

struct remote_domain_migrate_get_compression_cache_args { remote_nonnull_domain dom; u_int flags; @@ -3233,4 +3240,5 @@ enum remote_procedure { REMOTE_PROC_DOMAIN_SET_VCPU = 384, REMOTE_PROC_DOMAIN_EVENT_BLOCK_THRESHOLD = 385, REMOTE_PROC_DOMAIN_SET_BLOCK_THRESHOLD = 386, + REMOTE_PROC_DOMAIN_GET_MAX_DOWNTIME = 387

This needs to be: REMOTE_PROC_DOMAIN_MIGRATE_GET_MAX_DOWNTIME = 387, from check/syntax-check failure John

};

On 07/28/2017 10:57 AM, Scott Garfinkle wrote:

From: Scott Garfinkle <seg@us.ibm.com>

Again no commit message. Maybe a bit more verbose here indicating what's being added. You also need a followup patch 4 to docs/news.xml to adjust the 3.7.0 docs to describe the new feature briefly.

--- tools/virsh-domain.c | 46 ++++++++++++++++++++++++++++++++++++++++++++++ tools/virsh.pod | 18 ++++++++++++++++++ 2 files changed, 64 insertions(+)

diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index 0684979..10fdd0f 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -10720,6 +10720,46 @@ cmdMigrateSetMaxDowntime(vshControl *ctl, const vshCmd *cmd) }

/* + * "migrate-getmaxdowntime" command + */ +static const vshCmdInfo info_migrate_getmaxdowntime[] = { + {.name = "help", + .data = N_("get maximum tolerable downtime")

I note that get max speed capitalizes "get"...

+ }, + {.name = "desc", + .data = N_("Get maximum tolerable downtime (in milliseconds)for a domain which is being live-migrated to another host.")

Kind of long, but still there should be a space after the ')'. Not sure the text after "which ..." needs to be included in this section. Details like that can be in virsh.pod though. Also not sure it's worth it or not, but you could add a --seconds option to display in seconds too (kind of like --pretty for dubbas who cannot divide properly).

+ }, + {.name = NULL} +}; + +static const vshCmdOptDef opts_migrate_getmaxdowntime[] = { + VIRSH_COMMON_OPT_DOMAIN_FULL, + {.name = NULL} +}; + +static bool +cmdMigrateGetMaxDowntime(vshControl *ctl, const vshCmd *cmd) +{ + virDomainPtr dom = NULL; + unsigned long long downtime; + bool ret = false; + + if (!(dom = virshCommandOptDomain(ctl, cmd, NULL))) + return false; + + if (virDomainMigrateGetMaxDowntime(dom, &downtime, 0))

This should compare < 0 if (vir*(args) < 0)

+ goto done; + + vshPrint(ctl, "%llu\n", downtime); + + ret = true; + + done: + virshDomainFree(dom); + return ret; +} + +/* * "migrate-compcache" command */ static const vshCmdInfo info_migrate_compcache[] = { @@ -13845,6 +13885,12 @@ const vshCmdDef domManagementCmds[] = { .info = info_migrate_setmaxdowntime, .flags = 0 }, + {.name = "migrate-getmaxdowntime", + .handler = cmdMigrateGetMaxDowntime, + .opts = opts_migrate_getmaxdowntime, + .info = info_migrate_getmaxdowntime, + .flags = 0 + }, {.name = "migrate-compcache", .handler = cmdMigrateCompCache, .opts = opts_migrate_compcache, diff --git a/tools/virsh.pod b/tools/virsh.pod index 43d6f0c..fc0a46c 100644 --- a/tools/virsh.pod +++ b/tools/virsh.pod @@ -1869,6 +1869,24 @@ is supposed to be used while the domain is being live-migrated as a reaction to migration progress and increasing number of compression cache misses obtained from domjobinfo.

+=item B<migrate-getmaxdowntime> I<domain> + +Get the maximum tolerable downtime for a domain which is being live-migrated to +another host. This is the number of milliseconds the guest is allowed +to be down at the end of live migration. +

The next hunk needs to be a separate patch... either before or after this particular series as it's missing now.

+=item B<migrate-compcache> I<domain> [I<--size> B<bytes>] + +Sets and/or gets size of the cache (in bytes) used for compressing repeatedly +transferred memory pages during live migration. When called without I<size>, +the command just prints current size of the compression cache. When I<size> +is specified, the hypervisor is asked to change compression cache to I<size> +bytes and then the current size is printed (the result may differ from the

I believe here you should adjust "bytes" to be B<bytes>

+requested size due to rounding done by the hypervisor). The I<size> option +is supposed to be used while the domain is being live-migrated as a reaction

"is supposed to be used" makes it seem like it could be used outside of a migration. Perhaps better said, "is valid while"

+to migration progress and increasing number of compression cache misses

s/progress and increasing/progress. Increasing/ (e.g., remove the and) John

+obtained from domjobinfo. +

=item B<migrate-setspeed> I<domain> I<bandwidth>

Set the maximum migration bandwidth (in MiB/s) for a domain which is being