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

2017-08-14 Thread John Ferlan


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

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

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

> +requested size due to rounding done by the hypervisor). The I 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 I I
>  
>  Set the maximum migration bandwidth (in MiB/s) for a domain which is being
> 

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list


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

2017-07-28 Thread Scott Garfinkle
From: Scott Garfinkle 

---
 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")
+},
+{.name = "desc",
+ .data = N_("Get maximum tolerable downtime (in milliseconds)for a domain 
which is being live-migrated to another host.")
+},
+{.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))
+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 I
+
+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.
+
+=item B I [I<--size> B]
+
+Sets and/or gets size of the cache (in bytes) used for compressing repeatedly
+transferred memory pages during live migration. When called without I,
+the command just prints current size of the compression cache. When I
+is specified, the hypervisor is asked to change compression cache to I
+bytes and then the current size is printed (the result may differ from the
+requested size due to rounding done by the hypervisor). The I option
+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 I I
 
 Set the maximum migration bandwidth (in MiB/s) for a domain which is being
-- 
1.8.3.1

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list