[libvirt] Unbounded client streams

[Daniel P. Berrange]

On Tue, Apr 25, 2017 at 09:22:51AM +0200, Michal Privoznik wrote:
> Dear list,
> 
> as you might have seen, I've been playing with our streams lately. One of
> the things I wanted to try was how my sparse streams deal with vol-download
> and slow writer (e.g. slow disk). Note to whomever wants to try that out:
> blkio,throttle.write_* is no good for this. So I went the old way:

> And found out something ugly: the memory footprint of virsh just keeps
> growing as vol-download progresses. I've tracked down the problem to:
> 
> 1) The client event loop (virNetClientIOEventLoop) sees some incoming data,
> so it reads it (virNetClientIOHandleInput -> virNetClientCallDispatch) and
> queues the packet (virNetClientCallDispatchStream) regardless of the length
> of already queued packets
> 
> 2) Since the server sees client has read all the data, it sends even more
> data.

Yep, from the server's POV the current protocol design relies on the client
not consuming data if it is not ready to handle it.

There were two reasons for this. First for efficiency, we wanted to avoid
a round trip for every single block of data we wish to upload / download.
Second, and more importantly when using the streams for a bi-directional
interactive console, we've no idea when there will be data to download,
so we just wanted the server to send it as soon as it was received from
the guest.

> And the queue in step 1) just grows and grows. Ideally, the packets are
> taken out from the queue by virStreamRecv (which boils down to
> virNetClientStreamRecvPacket), but that is not on the schedule for the next
> minute.

Yep.

> Frankly, I don't have any idea how to fix this. We can stop reading the data
> (i.e. not set POLLIN on the client <-> server socket) if the queue reaches
> certain length. BUT, this would mean no other call is processed. Not even
> keepalive - and the connection would break.

My original intention was certainly that we would stop reading data off the
socket if we had too much queued, but this predated the existance of the
keepalive code.

I see three possible options (besides ignoring it)

 - Turn off the keepalive somehow when we want to pause reading from
   the stream
   
 - Somehow introduce stream "chunking". eg assume a chunk size of 10 MB
   is somehow enabled. The server would send 10 MB, and then not send
   any more data until the client issued a "continue" message of some
   kind, whereupon a further 10 MB is permitted to be sent.

 - Stop multiplexing streams on the socket. Have some way to open up
   additional sockets to the server, one per stream. The main connection
   would somehow provide a one time only auth token, so we don't
   need to force apps to go through an authentication step again.

Option 2 is probably less scary to retrofit.

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 :|