[libvirt] [PATCH v3 3/3] virsh: Add support for virDomainMigrateGetMaxDowntime

[John Ferlan]

On 07/28/2017 10:57 AM, Scott Garfinkle wrote:
> From: Scott Garfinkle <seg at 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
>