Message ID | 20221103102741.11201-3-davydov-max@yandex-team.ru (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | compare machine type compat_props | expand |
On Thu, Nov 3, 2022 at 6:29 AM Maksim Davydov <davydov-max@yandex-team.ru> 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 <davydov-max@yandex-team.ru> > Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> > --- > 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
On Tue, Nov 08, 2022 at 03:38:21PM -0500, John Snow wrote: > On Thu, Nov 3, 2022 at 6:29 AM Maksim Davydov > <davydov-max@yandex-team.ru> 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 <davydov-max@yandex-team.ru> > > Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> > > --- > > 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
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 > > <davydov-max@yandex-team.ru> 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 <davydov-max@yandex-team.ru> > > > Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> > > > --- > > > 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
On Wed, Nov 9, 2022, 6:00 AM Daniel P. Berrangé <berrange@redhat.com> 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 > > > <davydov-max@yandex-team.ru> 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 <davydov-max@yandex-team.ru> > > > > Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru > > > > > > --- > > > > 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 >
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]