[PATCH for 8.9.0] Document caveats of 'VIR_DOMAIN_STATS_VM' group of statistics

Peter Krempa posted 1 patch 1 year, 5 months ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/libvirt tags/patchew/1f4207247df6eeb3655ea7dafa57d79cb4bae062.1667298470.git.pkrempa@redhat.com
docs/manpages/virsh.rst      | 36 ++++++++++++++++++++++++++++++++++--
src/libvirt-domain.c         | 26 +++++++++++++++++++++++++-
tools/virsh-domain-monitor.c |  2 +-
3 files changed, 60 insertions(+), 4 deletions(-)
[PATCH for 8.9.0] Document caveats of 'VIR_DOMAIN_STATS_VM' group of statistics
Posted by Peter Krempa 1 year, 5 months ago
The original patches adding the functionality neglected to add any form
of documentation for the stats fields returned for this group.

The stats are directly converted from qemu's 'query-stats(-schema)' QMP
command without any further interpretation. The 'query-stats-schema' has
the following disclaimer:

 Note: runtime-collected statistics and their names fall outside QEMU's usual
       deprecation policies.  QEMU will try to keep the set of available data
       stable, together with their names, but will not guarantee stability
       at all costs; the same is true of providers that source statistics
       externally, e.g. from Linux.  For example, if the same value is being
       tracked with different names on different architectures or by different
       providers, one of them might be renamed.  A statistic might go away if
       an algorithm is changed or some code is removed; changing a default
       might cause previously useful statistics to always report 0.  Such
       changes, however, are expected to be rare.

Since libvirt is not doing any form of conversion of the stats we can't
meaningfully document any of the returned fields. At the same time we
can't even meaningfully provide any form of API stability for the filed
names.

Modify the documentation for the 'VIR_DOMAIN_STATS_VM' group both in the
API docs and in the virsh man page to reflect that and disclaim any form
of stability guarantees we provide normally.

Fixes: 8c9e3dae142
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
---
 docs/manpages/virsh.rst      | 36 ++++++++++++++++++++++++++++++++++--
 src/libvirt-domain.c         | 26 +++++++++++++++++++++++++-
 tools/virsh-domain-monitor.c |  2 +-
 3 files changed, 60 insertions(+), 4 deletions(-)

diff --git a/docs/manpages/virsh.rst b/docs/manpages/virsh.rst
index 61fcb2e9ca..cb2dbaec18 100644
--- a/docs/manpages/virsh.rst
+++ b/docs/manpages/virsh.rst
@@ -2297,7 +2297,7 @@ domstats

    domstats [--raw] [--enforce] [--backing] [--nowait] [--state]
       [--cpu-total] [--balloon] [--vcpu] [--interface]
-      [--block] [--perf] [--iothread] [--memory] [--dirtyrate]
+      [--block] [--perf] [--iothread] [--memory] [--dirtyrate] [--vm]
       [[--list-active] [--list-inactive]
        [--list-persistent] [--list-transient] [--list-running]y
        [--list-paused] [--list-shutoff] [--list-other]] | [domain ...]
@@ -2317,7 +2317,7 @@ The individual statistics groups are selectable via specific flags. By
 default all supported statistics groups are returned. Supported
 statistics groups flags are: *--state*, *--cpu-total*, *--balloon*,
 *--vcpu*, *--interface*, *--block*, *--perf*, *--iothread*, *--memory*,
-*--dirtyrate*.
+*--dirtyrate*, *--vm*.

 Note that - depending on the hypervisor type and version or the domain state
 - not all of the following statistics may be returned.
@@ -2533,6 +2533,38 @@ not available for statistical purposes.
 * ``dirtyrate.vcpu.<num>.megabytes_per_second`` - the calculated memory dirty
   rate for a virtual cpu in MiB/s

+*--vm* returns:
+
+The *--vm* option enables reporting of hypervisor-specific statistics. Naming
+and meaning of the fields is entirely hypervisor dependant.
+
+The statistics in this group have following naming scheme:
+
+ ``vm.$NAME.$TYPE``
+
+ ``$NAME``
+   name of the statistics field provided by the hypervisor
+
+ ``$TYPE``
+   Type of the value. Following types are returned:
+
+   ``cur``
+     current instant value
+   ``sum``
+     aggregate value
+   ``max``
+     peak value
+
+ The returned value may be either an unsigned long long or a boolean.
+
+ **WARNING**: The stats reported in this group are runtime-collected and
+ hypervisor originated, thus fall outside of the usual stable API
+ policies of libvirt.
+
+ Libvirt can't guarantee that the statistics reported from the outside
+ source will be present in further versions of the hypervisor, or that
+ naming or meaning will stay consistent. Changes to existing fields,
+ however, are expected to be rare.

 Selecting a specific statistics groups doesn't guarantee that the
 daemon supports the selected group of stats. Flag *--enforce*
diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c
index 52fa136186..4728ddd6ff 100644
--- a/src/libvirt-domain.c
+++ b/src/libvirt-domain.c
@@ -12482,7 +12482,31 @@ virConnectGetDomainCapabilities(virConnectPtr conn,
  *                                                   unsigned long long.
  *
  * VIR_DOMAIN_STATS_VM:
- *     Return fd-based KVM statistics for the target VM
+ *     Return hypervisor-specific statistics. Note that the naming and meaning
+ *     of the fields is entirely hypervisor dependant.
+ *
+ *     The statistics in this group have following naming scheme:
+ *
+ *     "vm.$NAME.$TYPE"
+ *
+ *       $NAME - name of the statistics field provided by the hypervisor
+ *
+ *       $TYPE - Type of the value. Following types are returned:
+ *          'cur' - current instant value
+ *          'sum' - aggregate value
+ *          'max' - peak value
+ *
+ *      The returned value may be either an unsigned long long or a boolean.
+ *
+ *     WARNING:
+ *      The stats reported in this group are runtime-collected and
+ *      hypervisor originated, thus fall outside of the usual stable API
+ *      policies of libvirt.
+ *
+ *      Libvirt can't guarantee that the statistics reported from the outside
+ *      source will be present in further versions of the hypervisor, or that
+ *      naming or meaning will stay consistent. Changes to existing fields,
+ *      however, are expected to be rare.
  *
  * Note that entire stats groups or individual stat fields may be missing from
  * the output in case they are not supported by the given hypervisor, are not
diff --git a/tools/virsh-domain-monitor.c b/tools/virsh-domain-monitor.c
index be8f827685..f89118f64f 100644
--- a/tools/virsh-domain-monitor.c
+++ b/tools/virsh-domain-monitor.c
@@ -2067,7 +2067,7 @@ static const vshCmdOptDef opts_domstats[] = {
     },
     {.name = "vm",
      .type = VSH_OT_BOOL,
-     .help = N_("report fd-based VM statistics by KVM"),
+     .help = N_("report hypervisor-specific statistics"),
     },
     {.name = "list-active",
      .type = VSH_OT_BOOL,
-- 
2.38.1
Re: [PATCH for 8.9.0] Document caveats of 'VIR_DOMAIN_STATS_VM' group of statistics
Posted by Jiri Denemark 1 year, 5 months ago
On Tue, Nov 01, 2022 at 11:28:11 +0100, Peter Krempa wrote:
> The original patches adding the functionality neglected to add any form
> of documentation for the stats fields returned for this group.
> 
> The stats are directly converted from qemu's 'query-stats(-schema)' QMP
> command without any further interpretation. The 'query-stats-schema' has
> the following disclaimer:
> 
>  Note: runtime-collected statistics and their names fall outside QEMU's usual
>        deprecation policies.  QEMU will try to keep the set of available data
>        stable, together with their names, but will not guarantee stability
>        at all costs; the same is true of providers that source statistics
>        externally, e.g. from Linux.  For example, if the same value is being
>        tracked with different names on different architectures or by different
>        providers, one of them might be renamed.  A statistic might go away if
>        an algorithm is changed or some code is removed; changing a default
>        might cause previously useful statistics to always report 0.  Such
>        changes, however, are expected to be rare.
> 
> Since libvirt is not doing any form of conversion of the stats we can't
> meaningfully document any of the returned fields. At the same time we
> can't even meaningfully provide any form of API stability for the filed
> names.
> 
> Modify the documentation for the 'VIR_DOMAIN_STATS_VM' group both in the
> API docs and in the virsh man page to reflect that and disclaim any form
> of stability guarantees we provide normally.
> 
> Fixes: 8c9e3dae142
> Signed-off-by: Peter Krempa <pkrempa@redhat.com>
> ---
>  docs/manpages/virsh.rst      | 36 ++++++++++++++++++++++++++++++++++--
>  src/libvirt-domain.c         | 26 +++++++++++++++++++++++++-
>  tools/virsh-domain-monitor.c |  2 +-
>  3 files changed, 60 insertions(+), 4 deletions(-)
> 
> diff --git a/docs/manpages/virsh.rst b/docs/manpages/virsh.rst
> index 61fcb2e9ca..cb2dbaec18 100644
> --- a/docs/manpages/virsh.rst
> +++ b/docs/manpages/virsh.rst
> @@ -2297,7 +2297,7 @@ domstats
> 
>     domstats [--raw] [--enforce] [--backing] [--nowait] [--state]
>        [--cpu-total] [--balloon] [--vcpu] [--interface]
> -      [--block] [--perf] [--iothread] [--memory] [--dirtyrate]
> +      [--block] [--perf] [--iothread] [--memory] [--dirtyrate] [--vm]
>        [[--list-active] [--list-inactive]
>         [--list-persistent] [--list-transient] [--list-running]y
>         [--list-paused] [--list-shutoff] [--list-other]] | [domain ...]
> @@ -2317,7 +2317,7 @@ The individual statistics groups are selectable via specific flags. By
>  default all supported statistics groups are returned. Supported
>  statistics groups flags are: *--state*, *--cpu-total*, *--balloon*,
>  *--vcpu*, *--interface*, *--block*, *--perf*, *--iothread*, *--memory*,
> -*--dirtyrate*.
> +*--dirtyrate*, *--vm*.
> 
>  Note that - depending on the hypervisor type and version or the domain state
>  - not all of the following statistics may be returned.
> @@ -2533,6 +2533,38 @@ not available for statistical purposes.
>  * ``dirtyrate.vcpu.<num>.megabytes_per_second`` - the calculated memory dirty
>    rate for a virtual cpu in MiB/s
> 
> +*--vm* returns:
> +
> +The *--vm* option enables reporting of hypervisor-specific statistics. Naming
> +and meaning of the fields is entirely hypervisor dependant.

s/dependant/dependent/ :-) twice, as the second copy in libvirt_domain.c
contains the same typo

> +
> +The statistics in this group have following naming scheme:

s/following/the following/ (twice)

> +
> + ``vm.$NAME.$TYPE``
> +
> + ``$NAME``
> +   name of the statistics field provided by the hypervisor
> +
> + ``$TYPE``
> +   Type of the value. Following types are returned:

s/Following/The following/ (twice)

> +
> +   ``cur``
> +     current instant value
> +   ``sum``
> +     aggregate value
> +   ``max``
> +     peak value
> +
> + The returned value may be either an unsigned long long or a boolean.
> +
> + **WARNING**: The stats reported in this group are runtime-collected and
> + hypervisor originated, thus fall outside of the usual stable API
> + policies of libvirt.
> +
> + Libvirt can't guarantee that the statistics reported from the outside
> + source will be present in further versions of the hypervisor, or that
> + naming or meaning will stay consistent. Changes to existing fields,
> + however, are expected to be rare.
> 
>  Selecting a specific statistics groups doesn't guarantee that the
>  daemon supports the selected group of stats. Flag *--enforce*
> diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c
> index 52fa136186..4728ddd6ff 100644
> --- a/src/libvirt-domain.c
> +++ b/src/libvirt-domain.c

The three corrections mentioned above need to be applied here as well.

Reviewed-by: Jiri Denemark <jdenemar@redhat.com>
Re: [PATCH for 8.9.0] Document caveats of 'VIR_DOMAIN_STATS_VM' group of statistics
Posted by Ján Tomko 1 year, 5 months ago
On a Tuesday in 2022, Peter Krempa wrote:
>The original patches adding the functionality neglected to add any form
>of documentation for the stats fields returned for this group.
>
>The stats are directly converted from qemu's 'query-stats(-schema)' QMP
>command without any further interpretation. The 'query-stats-schema' has
>the following disclaimer:
>
> Note: runtime-collected statistics and their names fall outside QEMU's usual
>       deprecation policies.  QEMU will try to keep the set of available data
>       stable, together with their names, but will not guarantee stability
>       at all costs; the same is true of providers that source statistics
>       externally, e.g. from Linux.  For example, if the same value is being
>       tracked with different names on different architectures or by different
>       providers, one of them might be renamed.  A statistic might go away if
>       an algorithm is changed or some code is removed; changing a default
>       might cause previously useful statistics to always report 0.  Such
>       changes, however, are expected to be rare.
>
>Since libvirt is not doing any form of conversion of the stats we can't
>meaningfully document any of the returned fields. At the same time we
>can't even meaningfully provide any form of API stability for the filed

*field

>names.
>
>Modify the documentation for the 'VIR_DOMAIN_STATS_VM' group both in the
>API docs and in the virsh man page to reflect that and disclaim any form
>of stability guarantees we provide normally.
>
>Fixes: 8c9e3dae142
>Signed-off-by: Peter Krempa <pkrempa@redhat.com>
>---
> docs/manpages/virsh.rst      | 36 ++++++++++++++++++++++++++++++++++--
> src/libvirt-domain.c         | 26 +++++++++++++++++++++++++-
> tools/virsh-domain-monitor.c |  2 +-
> 3 files changed, 60 insertions(+), 4 deletions(-)
>

Reviewed-by: Ján Tomko <jtomko@redhat.com>

Jano