[libvirt] [PATCH 00/23] Add block write threshold event

Peter Krempa posted 23 patches 7 years ago
Failed in applying to current master (apply log)
daemon/remote.c                                    |   45 +
examples/object-events/event-test.c                |   19 +
include/libvirt/libvirt-domain.h                   |   36 +
src/Makefile.am                                    |    1 +
src/conf/domain_event.c                            |   97 +
src/conf/domain_event.h                            |   15 +
src/driver-hypervisor.h                            |    8 +
src/libvirt-domain.c                               |   51 +
src/libvirt_private.syms                           |    5 +
src/libvirt_public.syms                            |    1 +
src/qemu/qemu_block.c                              |  378 ++++
src/qemu/qemu_block.h                              |   51 +
src/qemu/qemu_blockjob.c                           |    2 +
src/qemu/qemu_capabilities.c                       |    4 +
src/qemu/qemu_capabilities.h                       |    2 +
src/qemu/qemu_domain.c                             |  102 +
src/qemu/qemu_domain.h                             |   14 +
src/qemu/qemu_driver.c                             |   80 +-
src/qemu/qemu_monitor.c                            |   54 +-
src/qemu/qemu_monitor.h                            |   20 +
src/qemu/qemu_monitor_json.c                       |   91 +-
src/qemu/qemu_monitor_json.h                       |    9 +
src/qemu/qemu_process.c                            |   44 +
src/remote/remote_driver.c                         |   34 +
src/remote/remote_protocol.x                       |   33 +-
src/remote_protocol-structs                        |   16 +
src/util/virbuffer.c                               |   19 +
src/util/virbuffer.h                               |    2 +
src/util/virstoragefile.c                          |  108 +-
src/util/virstoragefile.h                          |   15 +
tests/qemucapabilitiesdata/caps_2.1.1.x86_64.xml   |    1 +
tests/qemucapabilitiesdata/caps_2.4.0.x86_64.xml   |    2 +
tests/qemucapabilitiesdata/caps_2.5.0.x86_64.xml   |    2 +
.../caps_2.6.0-gicv2.aarch64.xml                   |    2 +
.../caps_2.6.0-gicv3.aarch64.xml                   |    2 +
tests/qemucapabilitiesdata/caps_2.6.0.ppc64le.xml  |    2 +
tests/qemucapabilitiesdata/caps_2.6.0.x86_64.xml   |    2 +
tests/qemucapabilitiesdata/caps_2.7.0.s390x.xml    |    2 +
tests/qemucapabilitiesdata/caps_2.7.0.x86_64.xml   |    2 +
tests/qemucapabilitiesdata/caps_2.8.0.s390x.xml    |    2 +
tests/qemucapabilitiesdata/caps_2.8.0.x86_64.xml   |    2 +
tests/qemucapabilitiesdata/caps_2.9.0.x86_64.xml   |    2 +
.../qemumonitorjson-nodename-1.json                |  268 +++
.../qemumonitorjson-nodename-1.result              |   15 +
.../qemumonitorjson-nodename-2.json                | 2270 ++++++++++++++++++++
.../qemumonitorjson-nodename-2.result              |   60 +
.../qemumonitorjson-nodename-gluster.json          |  135 ++
.../qemumonitorjson-nodename-gluster.result        |   10 +
.../qemumonitorjson-nodename-relative.json         |  554 +++++
.../qemumonitorjson-nodename-relative.result       |   31 +
.../qemumonitorjson-nodename-same-backing.json     |  316 +++
.../qemumonitorjson-nodename-same-backing.result   |   11 +
tests/qemumonitorjsontest.c                        |  120 ++
tools/virsh-domain.c                               |   85 +
tools/virsh.pod                                    |    8 +
55 files changed, 5243 insertions(+), 19 deletions(-)
create mode 100644 src/qemu/qemu_block.c
create mode 100644 src/qemu/qemu_block.h
create mode 100644 tests/qemumonitorjsondata/qemumonitorjson-nodename-1.json
create mode 100644 tests/qemumonitorjsondata/qemumonitorjson-nodename-1.result
create mode 100644 tests/qemumonitorjsondata/qemumonitorjson-nodename-2.json
create mode 100644 tests/qemumonitorjsondata/qemumonitorjson-nodename-2.result
create mode 100644 tests/qemumonitorjsondata/qemumonitorjson-nodename-gluster.json
create mode 100644 tests/qemumonitorjsondata/qemumonitorjson-nodename-gluster.result
create mode 100644 tests/qemumonitorjsondata/qemumonitorjson-nodename-relative.json
create mode 100644 tests/qemumonitorjsondata/qemumonitorjson-nodename-relative.result
create mode 100644 tests/qemumonitorjsondata/qemumonitorjson-nodename-same-backing.json
create mode 100644 tests/qemumonitorjsondata/qemumonitorjson-nodename-same-backing.result
[libvirt] [PATCH 00/23] Add block write threshold event
Posted by Peter Krempa 7 years ago
This is another version of the stuff that I've posted here:
https://www.redhat.com/archives/libvir-list/2017-February/msg01391.html
which was partially based on the very old discussion at
https://www.redhat.com/archives/libvir-list/2015-May/msg00580.html

This version fixes some of the review feedback that I've got and also fixes all
the issues pointed out in the original cover letter since I managed to implement
the node name detection in a way suitable for this.

The event is useful for mgmt apps using thin-provisioned storage so that they
don't have to poll for the disk filling all the time.

Peter Krempa (23):
  qemu: driver: Don't call qemuDomainDetermineDiskChain on block jobs
  util: buffer: Add API to set indentation level to a given value
  util: storage: Split out useful bits of virStorageFileParseChainIndex
  util: storage: Add variables for node names into virStorageSource
  lib: Introduce event for tracking disk backing file write threshold
  qemu: monitor: Add support for BLOCK_WRITE_THRESHOLD event
  qemu: domain: Add helper to lookup disk by node name
  qemu: domain: Add helper to generate indexed backing store names
  qemu: process: Wire up firing of the
    VIR_DOMAIN_EVENT_ID_BLOCK_THRESHOLD event
  lib: Add API for setting the threshold size for
    VIR_DOMAIN_EVENT_ID_BLOCK_THRESHOLD
  virsh: Implement 'domblkthreshold' command to call
    virDomainSetBlockThreshold
  qemu: domain: Add helper to look up disk soruce by the backing store
    string
  qemu: implement qemuDomainSetBlockThreshold
  qemu: capabilities: add capability for query-named-block-nodes qmp cmd
  qemu: monitor: Add monitor infrastructure for query-named-block-nodes
  qemu: block: Add code to allow detection of auto-allocated node names
  tests: qemumonitorjson: Add test case for node name detection code
  tests: qemumonitorjson: Add long backing chain test case for node name
    detection
  tests: qemumonitorjson: Add case for two disks sharing a backing image
  tests: qemumonitorjson: Add relative image names for node name
    detection
  tests: qemumonitorjson: Test node name detection on networked storage
  qemu: monitor: Extract the top level format node when querying disks
  qemu: block: Add code to detect node names when necessary

 daemon/remote.c                                    |   45 +
 examples/object-events/event-test.c                |   19 +
 include/libvirt/libvirt-domain.h                   |   36 +
 src/Makefile.am                                    |    1 +
 src/conf/domain_event.c                            |   97 +
 src/conf/domain_event.h                            |   15 +
 src/driver-hypervisor.h                            |    8 +
 src/libvirt-domain.c                               |   51 +
 src/libvirt_private.syms                           |    5 +
 src/libvirt_public.syms                            |    1 +
 src/qemu/qemu_block.c                              |  378 ++++
 src/qemu/qemu_block.h                              |   51 +
 src/qemu/qemu_blockjob.c                           |    2 +
 src/qemu/qemu_capabilities.c                       |    4 +
 src/qemu/qemu_capabilities.h                       |    2 +
 src/qemu/qemu_domain.c                             |  102 +
 src/qemu/qemu_domain.h                             |   14 +
 src/qemu/qemu_driver.c                             |   80 +-
 src/qemu/qemu_monitor.c                            |   54 +-
 src/qemu/qemu_monitor.h                            |   20 +
 src/qemu/qemu_monitor_json.c                       |   91 +-
 src/qemu/qemu_monitor_json.h                       |    9 +
 src/qemu/qemu_process.c                            |   44 +
 src/remote/remote_driver.c                         |   34 +
 src/remote/remote_protocol.x                       |   33 +-
 src/remote_protocol-structs                        |   16 +
 src/util/virbuffer.c                               |   19 +
 src/util/virbuffer.h                               |    2 +
 src/util/virstoragefile.c                          |  108 +-
 src/util/virstoragefile.h                          |   15 +
 tests/qemucapabilitiesdata/caps_2.1.1.x86_64.xml   |    1 +
 tests/qemucapabilitiesdata/caps_2.4.0.x86_64.xml   |    2 +
 tests/qemucapabilitiesdata/caps_2.5.0.x86_64.xml   |    2 +
 .../caps_2.6.0-gicv2.aarch64.xml                   |    2 +
 .../caps_2.6.0-gicv3.aarch64.xml                   |    2 +
 tests/qemucapabilitiesdata/caps_2.6.0.ppc64le.xml  |    2 +
 tests/qemucapabilitiesdata/caps_2.6.0.x86_64.xml   |    2 +
 tests/qemucapabilitiesdata/caps_2.7.0.s390x.xml    |    2 +
 tests/qemucapabilitiesdata/caps_2.7.0.x86_64.xml   |    2 +
 tests/qemucapabilitiesdata/caps_2.8.0.s390x.xml    |    2 +
 tests/qemucapabilitiesdata/caps_2.8.0.x86_64.xml   |    2 +
 tests/qemucapabilitiesdata/caps_2.9.0.x86_64.xml   |    2 +
 .../qemumonitorjson-nodename-1.json                |  268 +++
 .../qemumonitorjson-nodename-1.result              |   15 +
 .../qemumonitorjson-nodename-2.json                | 2270 ++++++++++++++++++++
 .../qemumonitorjson-nodename-2.result              |   60 +
 .../qemumonitorjson-nodename-gluster.json          |  135 ++
 .../qemumonitorjson-nodename-gluster.result        |   10 +
 .../qemumonitorjson-nodename-relative.json         |  554 +++++
 .../qemumonitorjson-nodename-relative.result       |   31 +
 .../qemumonitorjson-nodename-same-backing.json     |  316 +++
 .../qemumonitorjson-nodename-same-backing.result   |   11 +
 tests/qemumonitorjsontest.c                        |  120 ++
 tools/virsh-domain.c                               |   85 +
 tools/virsh.pod                                    |    8 +
 55 files changed, 5243 insertions(+), 19 deletions(-)
 create mode 100644 src/qemu/qemu_block.c
 create mode 100644 src/qemu/qemu_block.h
 create mode 100644 tests/qemumonitorjsondata/qemumonitorjson-nodename-1.json
 create mode 100644 tests/qemumonitorjsondata/qemumonitorjson-nodename-1.result
 create mode 100644 tests/qemumonitorjsondata/qemumonitorjson-nodename-2.json
 create mode 100644 tests/qemumonitorjsondata/qemumonitorjson-nodename-2.result
 create mode 100644 tests/qemumonitorjsondata/qemumonitorjson-nodename-gluster.json
 create mode 100644 tests/qemumonitorjsondata/qemumonitorjson-nodename-gluster.result
 create mode 100644 tests/qemumonitorjsondata/qemumonitorjson-nodename-relative.json
 create mode 100644 tests/qemumonitorjsondata/qemumonitorjson-nodename-relative.result
 create mode 100644 tests/qemumonitorjsondata/qemumonitorjson-nodename-same-backing.json
 create mode 100644 tests/qemumonitorjsondata/qemumonitorjson-nodename-same-backing.result

-- 
2.12.0

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 00/23] Add block write threshold event
Posted by Peter Krempa 7 years ago
On Wed, Mar 15, 2017 at 17:37:12 +0100, Peter Krempa wrote:
> This is another version of the stuff that I've posted here:
> https://www.redhat.com/archives/libvir-list/2017-February/msg01391.html
> which was partially based on the very old discussion at
> https://www.redhat.com/archives/libvir-list/2015-May/msg00580.html
> 
> This version fixes some of the review feedback that I've got and also fixes all
> the issues pointed out in the original cover letter since I managed to implement
> the node name detection in a way suitable for this.
> 
> The event is useful for mgmt apps using thin-provisioned storage so that they
> don't have to poll for the disk filling all the time.

Michal's nvdimm series pushed recently caused a merge-conflict. You can
fetch the changes after I've resoved the conflict and with the two hunks
I forgot to commit before sending in my repo:

git fetch git://pipo.sk/pipo/libvirt.git block-threshold-1

Also I forgot to mention that I'm working on the ability to report the
currently set threshold via the bulk stats API. I'll post that
separately once I finish that.
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 00/23] Add block write threshold event
Posted by Peter Krempa 7 years ago
On Wed, Mar 15, 2017 at 17:37:12 +0100, Peter Krempa wrote:
> This is another version of the stuff that I've posted here:
> https://www.redhat.com/archives/libvir-list/2017-February/msg01391.html
> which was partially based on the very old discussion at
> https://www.redhat.com/archives/libvir-list/2015-May/msg00580.html
> 
> This version fixes some of the review feedback that I've got and also fixes all
> the issues pointed out in the original cover letter since I managed to implement
> the node name detection in a way suitable for this.
> 
> The event is useful for mgmt apps using thin-provisioned storage so that they
> don't have to poll for the disk filling all the time.
> 
> Peter Krempa (23):
>   qemu: driver: Don't call qemuDomainDetermineDiskChain on block jobs
>   util: buffer: Add API to set indentation level to a given value
>   util: storage: Split out useful bits of virStorageFileParseChainIndex
>   util: storage: Add variables for node names into virStorageSource
>   lib: Introduce event for tracking disk backing file write threshold
>   qemu: monitor: Add support for BLOCK_WRITE_THRESHOLD event
>   qemu: domain: Add helper to lookup disk by node name
>   qemu: domain: Add helper to generate indexed backing store names
>   qemu: process: Wire up firing of the
>     VIR_DOMAIN_EVENT_ID_BLOCK_THRESHOLD event
>   lib: Add API for setting the threshold size for
>     VIR_DOMAIN_EVENT_ID_BLOCK_THRESHOLD
>   virsh: Implement 'domblkthreshold' command to call
>     virDomainSetBlockThreshold
>   qemu: domain: Add helper to look up disk soruce by the backing store
>     string
>   qemu: implement qemuDomainSetBlockThreshold
>   qemu: capabilities: add capability for query-named-block-nodes qmp cmd
>   qemu: monitor: Add monitor infrastructure for query-named-block-nodes
>   qemu: block: Add code to allow detection of auto-allocated node names
>   tests: qemumonitorjson: Add test case for node name detection code
>   tests: qemumonitorjson: Add long backing chain test case for node name
>     detection
>   tests: qemumonitorjson: Add case for two disks sharing a backing image
>   tests: qemumonitorjson: Add relative image names for node name
>     detection
>   tests: qemumonitorjson: Test node name detection on networked storage
>   qemu: monitor: Extract the top level format node when querying disks
>   qemu: block: Add code to detect node names when necessary

Ping? This feature is very sought after by oVirt so that they can start
developing their code. I't be great if it would make this release.

Thanks.

Peter
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 00/23] Add block write threshold event
Posted by Eric Blake 7 years ago
On 03/22/2017 02:55 AM, Peter Krempa wrote:
> On Wed, Mar 15, 2017 at 17:37:12 +0100, Peter Krempa wrote:
>> This is another version of the stuff that I've posted here:
>> https://www.redhat.com/archives/libvir-list/2017-February/msg01391.html
>> which was partially based on the very old discussion at
>> https://www.redhat.com/archives/libvir-list/2015-May/msg00580.html
>>
>> This version fixes some of the review feedback that I've got and also fixes all
>> the issues pointed out in the original cover letter since I managed to implement
>> the node name detection in a way suitable for this.
>>
>> The event is useful for mgmt apps using thin-provisioned storage so that they
>> don't have to poll for the disk filling all the time.
>>

> 
> Ping? This feature is very sought after by oVirt so that they can start
> developing their code. I't be great if it would make this release.

I'll be starting my review today

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 00/23] Add block write threshold event
Posted by Peter Krempa 7 years ago
On Wed, Mar 15, 2017 at 17:37:12 +0100, Peter Krempa wrote:
> This is another version of the stuff that I've posted here:
> https://www.redhat.com/archives/libvir-list/2017-February/msg01391.html
> which was partially based on the very old discussion at
> https://www.redhat.com/archives/libvir-list/2015-May/msg00580.html
> 
> This version fixes some of the review feedback that I've got and also fixes all
> the issues pointed out in the original cover letter since I managed to implement
> the node name detection in a way suitable for this.
> 
> The event is useful for mgmt apps using thin-provisioned storage so that they
> don't have to poll for the disk filling all the time.

I've pushed the updated version after incorporating most of the review
feedback to:

git fetch git://pipo.sk/pipo/libvirt.git block-threshold-2
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 00/23] Add block write threshold event
Posted by Eric Blake 7 years ago
On 03/24/2017 09:55 AM, Peter Krempa wrote:
> On Wed, Mar 15, 2017 at 17:37:12 +0100, Peter Krempa wrote:
>> This is another version of the stuff that I've posted here:
>> https://www.redhat.com/archives/libvir-list/2017-February/msg01391.html
>> which was partially based on the very old discussion at
>> https://www.redhat.com/archives/libvir-list/2015-May/msg00580.html
>>
>> This version fixes some of the review feedback that I've got and also fixes all
>> the issues pointed out in the original cover letter since I managed to implement
>> the node name detection in a way suitable for this.
>>
>> The event is useful for mgmt apps using thin-provisioned storage so that they
>> don't have to poll for the disk filling all the time.
> 
> I've pushed the updated version after incorporating most of the review
> feedback to:
> 
> git fetch git://pipo.sk/pipo/libvirt.git block-threshold-2
> 

Here's the interdiff; still a couple of things to tweak:

> diff --git w/src/libvirt-domain.c c/src/libvirt-domain.c
> index 815cbb1..ca63138 100644
> --- w/src/libvirt-domain.c
> +++ c/src/libvirt-domain.c
> @@ -11838,8 +11838,12 @@ virDomainSetVcpu(virDomainPtr domain,
>   * Set the threshold level for delivering the
>   * VIR_DOMAIN_EVENT_ID_BLOCK_THRESHOLD if the device or backing chain element
>   * described by @dev is written beyond the set threshold level. The threshold
> - * level is unset once the event fired. The event may not be delivered at all if
> - * libvirtd was not running at the moment when the threshold was reached.
> + * level is unset once the event fired. The event might not be delivered at all

maybe s/fired/fires/

> + * if libvirtd was not running at the moment when the threshold was reached.
> + *
> + * Hypervisors report the last  written sector of an image in the bulk stats API

extra space

> + * (virConnectGetAllDomainStats/virDomainListGetStats) as
> + * "block.<num>.allocation" in the VIR_DOMAIN_STATS_BLOCK group.

Maybe also mention (thanks to the followup patch) that
block.<num>.threshold can be inspected in bulk stats to learn the
current threshold.

>   *
>   * This event allows to use thin-provisioned storage which needs management
>   * tools to grow it without the need for polling of the data.
> diff --git w/src/qemu/qemu_domain.c c/src/qemu/qemu_domain.c
> index 0469a1f..6985643 100644
> --- w/src/qemu/qemu_domain.c
> +++ c/src/qemu/qemu_domain.c
> @@ -8521,7 +8521,7 @@ qemuDomainNamespaceTeardownRNG(virQEMUDriverPtr driver,
>   * @src: filled with the specific backing store element if provided
>   * @idx: index of @src in the backing chain, if provided
>   *
> - * Looks up the disk in the domain via @nodename and returns it's definition.
> + * Looks up the disk in the domain via @nodename and returns its definition.
>   * Optionally fills @src and @idx if provided with the specific backing chain
>   * element which corresponds to the node name.
>   */
> diff --git w/src/qemu/qemu_process.c c/src/qemu/qemu_process.c
> index 8477710..53b4c5f 100644
> --- w/src/qemu/qemu_process.c
> +++ c/src/qemu/qemu_process.c
> @@ -1470,6 +1470,7 @@ qemuProcessHandleBlockThreshold(qemuMonitorPtr mon ATTRIBUTE_UNUSED,
>          if ((dev = qemuDomainDiskBackingStoreGetName(disk, src, idx))) {
>              event = virDomainEventBlockThresholdNewFromObj(vm, dev, path,
>                                                             threshold, excess);
> +            VIR_FREE(dev);
>          }

Oh nice - you caught something I missed.

>      }
> 
> diff --git w/src/remote/remote_driver.c c/src/remote/remote_driver.c
> index baa5cba..27bcc9b 100644
> --- w/src/remote/remote_driver.c
> +++ c/src/remote/remote_driver.c
> @@ -8436,7 +8436,7 @@ static virHypervisorDriver hypervisor_driver = {
>      .domainGetGuestVcpus = remoteDomainGetGuestVcpus, /* 2.0.0 */
>      .domainSetGuestVcpus = remoteDomainSetGuestVcpus, /* 2.0.0 */
>      .domainSetVcpu = remoteDomainSetVcpu, /* 3.1.0 */
> -    .domainSetBlockThreshold = remoteDomainSetBlockThreshold, /* 3.1.0 */
> +    .domainSetBlockThreshold = remoteDomainSetBlockThreshold, /* 3.2.0 */
>  };
> 
>  static virNetworkDriver network_driver = {
> diff --git w/src/util/virbuffer.c c/src/util/virbuffer.c
> index 8f0f49d..80c8e28 100644
> --- w/src/util/virbuffer.c
> +++ c/src/util/virbuffer.c
> @@ -90,7 +90,7 @@ virBufferAdjustIndent(virBufferPtr buf, int indent)
> 
> 
>  /**
> - * virBufferAdjustIndent:
> + * virBufferSetIndent:
>   * @buf: the buffer
>   * @indent: new indentation size.
>   *
> diff --git w/tests/virbuftest.c c/tests/virbuftest.c
> index 34160e6..8ec6ce5 100644
> --- w/tests/virbuftest.c
> +++ c/tests/virbuftest.c
> @@ -405,6 +405,34 @@ testBufEscapeN(const void *opaque)
> 
> 
>  static int
> +testBufSetIndent(const void *opaque ATTRIBUTE_UNUSED)
> +{

I might have just squashed in a test with the existing tests of
AdjustIndent, but what you have is fine.

> +    virBuffer buf = VIR_BUFFER_INITIALIZER;
> +    char *actual;
> +    int ret = -1;
> +
> +    virBufferSetIndent(&buf, 11);
> +    virBufferAddLit(&buf, "test\n");
> +    virBufferSetIndent(&buf, 2);
> +    virBufferAddLit(&buf, "test2\n");
> +
> +    if (!(actual = virBufferContentAndReset(&buf)))
> +        goto cleanup;
> +
> +    if (STRNEQ(actual, "           test\n  test2\n")) {
> +        VIR_TEST_DEBUG("testBufSetIndent: expected indent not set\n");
> +        goto cleanup;
> +    }
> +
> +    ret = 0;
> +
> + cleanup:
> +    VIR_FREE(actual);
> +    return ret;
> +}
> +
> +
> +static int
>  mymain(void)
>  {
>      int ret = 0;
> @@ -422,6 +450,7 @@ mymain(void)
>      DO_TEST("Auto-indentation", testBufAutoIndent, 0);
>      DO_TEST("Trim", testBufTrim, 0);
>      DO_TEST("AddBuffer", testBufAddBuffer, 0);
> +    DO_TEST("set indent", testBufSetIndent, 0);
> 
>  #define DO_TEST_ADD_STR(DATA, EXPECT)                                  \
>      do {                                                               \
> diff --git w/tools/virsh-domain.c c/tools/virsh-domain.c
> index 3662952..74d0a0e 100644
> --- w/tools/virsh-domain.c
> +++ c/tools/virsh-domain.c
> @@ -7105,7 +7105,7 @@ static const vshCmdInfo info_domblkthreshold[] = {
>                  "device or it's backing chain element")
>      },
>      {.name = "desc",
> -     .data = N_("set threshold for ")
> +     .data = N_("set threshold for block-threshold even for a block device")

s/even/event/

>      },
>      {.name = NULL}
>  };

Once you make the right tweaks in the corresponding patches, I think
that means you've earned an ACK to the series.  My next email should be
a summary of my testing...

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 00/23] Add block write threshold event
Posted by Eric Blake 7 years ago
On 03/24/2017 12:45 PM, Eric Blake wrote:

>>> The event is useful for mgmt apps using thin-provisioned storage so that they
>>> don't have to poll for the disk filling all the time.
>>
>> I've pushed the updated version after incorporating most of the review
>> feedback to:
>>
>> git fetch git://pipo.sk/pipo/libvirt.git block-threshold-2
>>
> 
> Here's the interdiff; still a couple of things to tweak:
> 

> Once you make the right tweaks in the corresponding patches, I think
> that means you've earned an ACK to the series.  My next email should be
> a summary of my testing...

First round of testing: new virsh, old libvirtd:

# tools/virsh domblkthreshold mydom
error: command 'domblkthreshold' requires <dev> option
error: command 'domblkthreshold' requires --threshold option

Weird that we aren't consistent in reporting the missing option name
(<dev> vs. --threshold); but it appears to be based on the type the
option is expecting, and not the fault of your series, so a fix (if any)
would be for a followup, and doesn't block your series.

# tools/virsh domblkthreshold mydom vdb 1m
error: unknown procedure: 386

It might be nicer if we could teach the remote driver (on the client
side) to augment unknown procedure errors to additionally include the
procedure name it thought it was calling, but again not the fault of
your series.

# tools/virsh event --list
...
metadata-change
block-threshold

Good - virsh displays the new event to register for.

# tools/virsh event --domain mydom --event block-threshold
error: internal error: unsupported event ID 24

Urggh - what's that "internal error:" doing in there? We should fix
that. But again, I don't think it's the fault of your series, and can be
done as a followup.


Next round of testing: new virsh, new libvirtd, Fedora 25
fedora-virt-preview's qemu-kvm-2.9.0-0.1.rc1.fc25.x86_64:

First, I validated that my domain is set up to spot watermark growth (it
requires a qcow2 image), merely booting the guest is enough to see that
writes start happening to the guest's view of the image, all as part of
mounting the disk prior to even showing the login screen (I specifically
am testing with vdb, with the guest OS installed in vda, so that the
file system on vdb is less heavily used by the OS and therefore a bit
more predictable in behavior as I reboot the guest):

# tools/virsh start mydom --paused
Domain mydom started
# tools/virsh domstats mydom --block
Domain: 'mydom'
  block.count=2
...
  block.1.allocation=0
  block.1.capacity=21474836480
  block.1.physical=21480095744
# tools/virsh resume mydom
Domain mydom resumed
# sleep 10
# tools/virsh domstats mydom --block
Domain: 'mydom'
  block.count=2
...
  block.1.allocation=10878128640
  block.1.capacity=21474836480
  block.1.physical=21480095744
# tools/virsh shutdown mydom
Domain mydom is being shutdown


block.1.allocation does indeed grow (and I repeated this test a couple
of times to validate that my particular guest OS has the same allocation
upon reaching the login screen), so the next step is to see if I can
register a threshold.  I'm going to pick something near where the OS
probes (note that I'm starting the domain paused so that I'm guaranteed
my threshold won't fire until the guest is resumed):

# tools/virsh domblkthreshold mydom vdb 10GB
error: Operation not supported: this qemu does not support setting
device threshold

That's a bit misleading. In this case, we don't support setting device
threshold because the domain is not running (there is no qemu process at
all when it is shutdown).  But at least you correctly failed to set a
threshold in this state.

# tools/virsh start mydom --paused
Domain mydom started
# tools/virsh domblkthreshold mydom vdb 9GB

# tools/virsh domblkthreshold mydom vdb 10GB

Nothing at all printed. But that's okay (I don't think we have to always
be chatty on success); and at least we can register a new value even if
an old value is already in place and has not yet fired.

# tools/virsh domstats mydom --block
Domain: 'mydom'
  block.count=2
...
  block.1.allocation=0
  block.1.capacity=21474836480
  block.1.physical=21480095744
  block.1.threshold.storage=10000000000

Yay - the new block.1.threshold.storage stat matches my most-recent
registration! (Remember, this is just testing the current state of your
series; if someone repeats these steps for validation later on, and you
choose to tweak it to a shorter name for the final commit, then be
prepared for a slight difference in output)

# tools/virsh event --domain mydom --event block-threshold &
[1] 11351
# tools/virsh resume mydom
Domain mydom resumed
event 'block-threshold' for domain mydom: dev: vdb(/path/to/vdb.qcow2)
10000000000 880225792
events received: 1
[1]+  Done                    tools/virsh event --domain mydom --event
block-threshold
# tools/virsh domstats mydom --block
Domain: 'mydom'
  block.count=2
...
  block.1.allocation=10880225792
  block.1.capacity=21474836480
  block.1.physical=21480095744

Would you look at that: block.1.threshold.storage disappeared now that
the event fired; and the new block.1.allocation matches the sum of the
threshold and the excess as reported in the event (meaning my OS did a
single far-flung write beyond the 10GB mark to cause the event).  I
think we can declare success that the watermark event worked!


So overall, it looks like you wired things up nicely, and I wasn't able
to cause any major problems in my testing, even if we can improve some
of the error message.  Looks good to go, and you should be able to get
it in before DV freezes in preparation for the release.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 00/23] Add block write threshold event
Posted by Peter Krempa 7 years ago
On Fri, Mar 24, 2017 at 17:09:47 -0500, Eric Blake wrote:
> On 03/24/2017 12:45 PM, Eric Blake wrote:
> 
> >>> The event is useful for mgmt apps using thin-provisioned storage so that they
> >>> don't have to poll for the disk filling all the time.
> >>
> >> I've pushed the updated version after incorporating most of the review
> >> feedback to:
> >>
> >> git fetch git://pipo.sk/pipo/libvirt.git block-threshold-2
> >>
> > 
> > Here's the interdiff; still a couple of things to tweak:
> > 
> 
> > Once you make the right tweaks in the corresponding patches, I think
> > that means you've earned an ACK to the series.  My next email should be
> > a summary of my testing...

Thanks. I've fixed the stuff you pointed out and I'll try to look into
the few pre-existing problems you've pointed out. This series is now
pushed.

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