[v3,2/4] python/qmp: increase read buffer size

Commit Message

Maksim Davydov Nov. 3, 2022, 10:27 a.m. UTC

After modification of "query-machines" command the buffer size should be
more than 452kB to contain output with compat-props.

Signed-off-by: Maksim Davydov <[e-mail]>
Reviewed-by: Vladimir Sementsov-Ogievskiy <[e-mail]>
---
 python/qemu/qmp/qmp_client.py | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Comments

John Snow Nov. 8, 2022, 8:38 p.m. UTC | #1

On Thu, Nov 3, 2022 at 6:29 AM Maksim Davydov
<[e-mail]> wrote:
>
> After modification of "query-machines" command the buffer size should be
> more than 452kB to contain output with compat-props.
>
> Signed-off-by: Maksim Davydov <[e-mail]>
> Reviewed-by: Vladimir Sementsov-Ogievskiy <[e-mail]>
> ---
>  python/qemu/qmp/qmp_client.py | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/python/qemu/qmp/qmp_client.py b/python/qemu/qmp/qmp_client.py
> index 5dcda04a75..659fe4d98c 100644
> --- a/python/qemu/qmp/qmp_client.py
> +++ b/python/qemu/qmp/qmp_client.py
> @@ -197,8 +197,8 @@ async def run(self, address='/tmp/qemu.socket'):
>      #: Logger object used for debugging messages.
>      logger = logging.getLogger(__name__)
>
> -    # Read buffer limit; large enough to accept query-qmp-schema
> -    _limit = (256 * 1024)
> +    # Read buffer limit; large enough to accept query-machines
> +    _limit = (512 * 1024)

wow :)

>
>      # Type alias for pending execute() result items
>      _PendingT = Union[Message, ExecInterruptedError]
> --
> 2.25.1
>

If you would please submit this to
https://gitlab.com/qemu-project/python-qemu-qmp I can get it merged
there quickly, then backport it to qemu.git.
Or, if you don't have a gitlab account (and do not want one), please
let me know and I'll port it there myself so you don't have to.

thanks,
--js

Daniel P. Berrangé Nov. 9, 2022, 9:39 a.m. UTC | #2

On Tue, Nov 08, 2022 at 03:38:21PM -0500, John Snow wrote:
> On Thu, Nov 3, 2022 at 6:29 AM Maksim Davydov
> <[e-mail]> wrote:
> >
> > After modification of "query-machines" command the buffer size should be
> > more than 452kB to contain output with compat-props.
> >
> > Signed-off-by: Maksim Davydov <[e-mail]>
> > Reviewed-by: Vladimir Sementsov-Ogievskiy <[e-mail]>
> > ---
> >  python/qemu/qmp/qmp_client.py | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/python/qemu/qmp/qmp_client.py b/python/qemu/qmp/qmp_client.py
> > index 5dcda04a75..659fe4d98c 100644
> > --- a/python/qemu/qmp/qmp_client.py
> > +++ b/python/qemu/qmp/qmp_client.py
> > @@ -197,8 +197,8 @@ async def run(self, address='/tmp/qemu.socket'):
> >      #: Logger object used for debugging messages.
> >      logger = logging.getLogger(__name__)
> >
> > -    # Read buffer limit; large enough to accept query-qmp-schema
> > -    _limit = (256 * 1024)
> > +    # Read buffer limit; large enough to accept query-machines
> > +    _limit = (512 * 1024)
> 
> wow :)

Meanwhile over in python/qemu/qmp/protocol.py the read buffer limit is
set to just 64 kb.

If the current output of a particular command is known to 450 kb, then
setting this limit to 512 kb is waaaaaaay to conservative, and we'll
inevitably have to change it again when someone finds the next command
that overflows.

Recall this thread

   https://lists.gnu.org/archive/html/qemu-devel/2022-09/msg01060.html

In fact, let me be the someone who demonstrates a real case where 512kb
is not enough....

Create a 200 deep chain of qcow2 files

   qemu-img create -f qcow2 demo0.img 1G
   j=0
   for i in `seq 1 200`
   do
     qemu-img create -f qcow2 -o backing_file=demo$j.qcow2 \
                              -o backing_fmt=qcow2 demo$i.qcow2
     j=$i
   done

Launch qemu with that, and then run

  "query-named-block-nodes"

and (many minutes later) you'll get an 86 MB  QMP reply.

Now, a bare no-arg "query-named-block-nodes" is known to be a bad
command as it returns data which is a combinatorial expansion of
the number of block nodes.  Essentially it is broken by design,
and no one should use it, without setting flat=true.


If we repeat it with flat=true, then we can get a 2.7 MB QMP
reply.  This is large, but a valid reply. Basically 13 KB of
data per qcow2 file.

Libvirt caps the backing chain depth it is willing to accept at 200
qcow2 files, but allows multiple disks. So with 4 disks, each with
200 deep chain, you'll get 10.8 MB of json back. Opps....

...Libvirt's QMP reply limit is 10 MB, so even libvirt will break
quite quickly. Libvirt can cope with around 787 qcow2 files in
at 13 kb per file. NB I'm assuming paths under the dir
/var/lib/libvirt/images/, shorter paths will allow for more.


The 64 KB limit will only cope with 4 qcow2 files before breaking,
while a 512 KB limit will only cope with 39 files before breaking.
Neither is anywhere near sufficient.

I'd suggest following libvirt here and setting the read limits to
10 MB.

With regards,
Daniel

Daniel P. Berrangé Nov. 9, 2022, 10:59 a.m. UTC | #3

On Wed, Nov 09, 2022 at 09:39:14AM +0000, Daniel P. Berrangé wrote:
> On Tue, Nov 08, 2022 at 03:38:21PM -0500, John Snow wrote:
> > On Thu, Nov 3, 2022 at 6:29 AM Maksim Davydov
> > <[e-mail]> wrote:
> > >
> > > After modification of "query-machines" command the buffer size should be
> > > more than 452kB to contain output with compat-props.
> > >
> > > Signed-off-by: Maksim Davydov <[e-mail]>
> > > Reviewed-by: Vladimir Sementsov-Ogievskiy <[e-mail]>
> > > ---
> > >  python/qemu/qmp/qmp_client.py | 4 ++--
> > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/python/qemu/qmp/qmp_client.py b/python/qemu/qmp/qmp_client.py
> > > index 5dcda04a75..659fe4d98c 100644
> > > --- a/python/qemu/qmp/qmp_client.py
> > > +++ b/python/qemu/qmp/qmp_client.py
> > > @@ -197,8 +197,8 @@ async def run(self, address='/tmp/qemu.socket'):
> > >      #: Logger object used for debugging messages.
> > >      logger = logging.getLogger(__name__)
> > >
> > > -    # Read buffer limit; large enough to accept query-qmp-schema
> > > -    _limit = (256 * 1024)
> > > +    # Read buffer limit; large enough to accept query-machines
> > > +    _limit = (512 * 1024)
> > 
> > wow :)
> 
> Meanwhile over in python/qemu/qmp/protocol.py the read buffer limit is
> set to just 64 kb.
> 
> If the current output of a particular command is known to 450 kb, then
> setting this limit to 512 kb is waaaaaaay to conservative, and we'll
> inevitably have to change it again when someone finds the next command
> that overflows.
> 
> Recall this thread
> 
>    https://lists.gnu.org/archive/html/qemu-devel/2022-09/msg01060.html
> 
> In fact, let me be the someone who demonstrates a real case where 512kb
> is not enough....

Another example...

Create a guest with 255 vCPUs (current RHEL downstream vCPU limit),
and run

  {"execute":"query-stats","arguments":{"target": "vcpu"}}

it'll get back a 0.38 MB  QMP reply.  RHEL raised the limit to 710
vCPUs, giving a little over 1 MB QMP reply. There is a strong desire
to go even higher. With 4096 vCPUs it'd get an ~6 MB QMP reply.

With regards,
Daniel

John Snow Nov. 9, 2022, 5:53 p.m. UTC | #4

On Wed, Nov 9, 2022, 6:00 AM Daniel P. Berrangé <[e-mail]> wrote:

> On Wed, Nov 09, 2022 at 09:39:14AM +0000, Daniel P. Berrangé wrote:
> > On Tue, Nov 08, 2022 at 03:38:21PM -0500, John Snow wrote:
> > > On Thu, Nov 3, 2022 at 6:29 AM Maksim Davydov
> > > <[e-mail]> wrote:
> > > >
> > > > After modification of "query-machines" command the buffer size
> should be
> > > > more than 452kB to contain output with compat-props.
> > > >
> > > > Signed-off-by: Maksim Davydov <[e-mail]>
> > > > Reviewed-by: Vladimir Sementsov-Ogievskiy <[e-mail]
> >
> > > > ---
> > > >  python/qemu/qmp/qmp_client.py | 4 ++--
> > > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/python/qemu/qmp/qmp_client.py
> b/python/qemu/qmp/qmp_client.py
> > > > index 5dcda04a75..659fe4d98c 100644
> > > > --- a/python/qemu/qmp/qmp_client.py
> > > > +++ b/python/qemu/qmp/qmp_client.py
> > > > @@ -197,8 +197,8 @@ async def run(self, address='/tmp/qemu.socket'):
> > > >      #: Logger object used for debugging messages.
> > > >      logger = logging.getLogger(__name__)
> > > >
> > > > -    # Read buffer limit; large enough to accept query-qmp-schema
> > > > -    _limit = (256 * 1024)
> > > > +    # Read buffer limit; large enough to accept query-machines
> > > > +    _limit = (512 * 1024)
> > >
> > > wow :)
> >
> > Meanwhile over in python/qemu/qmp/protocol.py the read buffer limit is
> > set to just 64 kb.
>

This one will override the other - the protocol limit is for any arbitrary
full-duplex message-based protocol. It can stay at the lower limit.

(I used protocol.py to implement a qtest client as well, though I didn't
upstream that piece. If there's interest, I will.)

>
> > If the current output of a particular command is known to 450 kb, then
> > setting this limit to 512 kb is waaaaaaay to conservative, and we'll
> > inevitably have to change it again when someone finds the next command
> > that overflows.
> >
> > Recall this thread
> >
> >    https://lists.gnu.org/archive/html/qemu-devel/2022-09/msg01060.html
> >
> > In fact, let me be the someone who demonstrates a real case where 512kb
> > is not enough....
>
> Another example...
>
> Create a guest with 255 vCPUs (current RHEL downstream vCPU limit),
> and run
>
>   {"execute":"query-stats","arguments":{"target": "vcpu"}}
>
> it'll get back a 0.38 MB  QMP reply.  RHEL raised the limit to 710
> vCPUs, giving a little over 1 MB QMP reply. There is a strong desire
> to go even higher. With 4096 vCPUs it'd get an ~6 MB QMP reply.
>
> With regards,
> Daniel
> --
> |: https://berrange.com      -o-
> https://www.flickr.com/photos/dberrange :|
> |: https://libvirt.org         -o-
> https://fstop138.berrange.com :|
> |: https://entangle-photo.org    -o-
> https://www.instagram.com/dberrange :|
>

You're right, of course. I recalled the thread but I was being lazy about
it. (Sorry.) I thought, naively, that it was better to speed Maksim along
for now.

I recall you (Daniel) stating that libvirt used a default of 10MB (iirc).
I'd be happy to adopt that default as well, if only for parity.

Maksim, can I trouble you to send a revised patch as an MR to
gitlab.com/qemu-project/python-qemu-qmp ? If not, a revised patch to the
mailing list here is fine and with your permission I'll forward-port it
over to the standalone repo myself. (Or I can just handle it entirely
myself, if you'd prefer - just let me know.)

Sorry for the fuss, and thanks for helping to improve QMP testing and
tooling

--js

>

Patch

diff --git a/python/qemu/qmp/qmp_client.py b/python/qemu/qmp/qmp_client.py
index 5dcda04a75..659fe4d98c 100644
--- a/python/qemu/qmp/qmp_client.py
+++ b/python/qemu/qmp/qmp_client.py
@@ -197,8 +197,8 @@  async def run(self, address='/tmp/qemu.socket'):
     #: Logger object used for debugging messages.
     logger = logging.getLogger(__name__)
 
-    # Read buffer limit; large enough to accept query-qmp-schema
-    _limit = (256 * 1024)
+    # Read buffer limit; large enough to accept query-machines
+    _limit = (512 * 1024)
 
     # Type alias for pending execute() result items
     _PendingT = Union[Message, ExecInterruptedError]