[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [Libguestfs] Progress bars



On Fri, Jul 02, 2010 at 11:27:26PM +0100, Richard W.M. Jones wrote:
> Background
> ----------
> 
> A complaint I'm hearing is that some tools which take a long time to
> run (virt-resize in particular) need to have progress bars to indicate
> how long they are expected to run.  This is also a basic usability
> principle, see for example this paper:
> http://www.chrisharrison.net/projects/progressbars/ProgBarHarrison.pdf
> 
> If you look at how virt-resize is implemented, the bulk of the time is
> spent copying partitions in a single long-running function, copy_size:
> 
>   http://libguestfs.org/guestfs.3.html#guestfs_copy_size
> 
> The current protocol is entirely synchronous so that while the daemon
> is doing the copy_size, the library is blocked waiting for a reply
> message and does nothing else in that thread.  To implement a progress
> bar for this call we'd have to have a way to query into the call while
> it was running to find out how far it has got, or to have it send
> regular status messages out.  The plan below outlines a way to do
> this.
> 
> Protocol changes
> ----------------
> 
> This plan does require changing the protocol.  We're allowed to change
> the library/daemon protocol (it's not ABI) but we tend to avoid doing
> it because it means people can't repackage our appliance and use it in
> other distributions (see http://libguestfs.org/FAQ.html#distros).  So
> if we do change it now, we should:
> 
> (a) Only do it at the start of the 1.5 development cycle, indicating
> that now would be a good time to release 1.4.  See:
> https://www.redhat.com/archives/libguestfs/2010-June/msg00069.html
> 
> (b) We should make the other protocol changes we've been wanting to
> do, see: http://libguestfs.org/guestfs.3.html#libguestfs_gotchas
> 
> (c) We need to make sure everyone understands the change to the
> appliance and protocol.



> 
> The current protocol (ignoring file transfers) implements a very
> simple RPC mechanism which is entirely synchronous:
> 
>   library                           daemon
>     request message --------------->
>                              daemon processes message
>     <----------------- reply message
> 
> I have discarded the idea that we should change to using an
> asynchronous system, eg. allowing the library to issue more than one
> request message simultaneously, because at this stage it adds great
> complexity to both ends, and is I believe not necessary in order to
> implement progress messages.  This means that we can't make additional
> "get status" requests during a call.

Allowing overlapping parallel requests isn't really an asychronous
system, at least from the protocol POV. It could be from the daemon
implementation POV if the daemon didn't want to use threads for RPC
dispatch. 

In libvirt we didn't make the individual API calls asynchronous,
but we did allow for multiple threads to use a single connection
concurrently, so one thread doesn't block another at a protocol
level. It certainly added alot of complexity to the code, but
becoming fully multithread capable was a big win

> Instead I'd like to change the protocol like this:
> 
>   library                           daemon
>     request message --------------->
>                              daemon processes message
>     <---------------- status message
>                              daemon processes message
>     <---------------- status message
>                              daemon processes message
>     <----------------- reply message
> 
> In a long running call such as copy_size, the daemon would send
> periodic status messages to the library.
> 
> Points to note:
> 
> (i) The daemon should self-limit these messages, eg. to once per
> second, starting at least one second after the request.
> 
> (ii) An initial implementation of the library would simply discard
> these status messages.  This would allow us to test the daemon side,
> only making trivial library changes, and be reasonably sure that the
> daemon side is correct.
> 
> (iii) The status message contains just two (64 bit) numbers:
> 
>   - Total size of the current operation
> 
>   - Current progress (0 <= current <= total size)
> 
>   The meaning of these two numbers is defined by the context of the
>   call, but would usually indicate, eg. total size in bytes and number
>   of bytes processed/copied so far.  Callers are only interested in
>   the ratio of these two numbers when displaying a progress meter.

This need not be incompatible. The async status message packets can 
use a new value for 'guest_message_direction'. If existing clients
don't already ignore packets with unknown direction field, then you
could include a post-connect RPC call to turn on the status messages
for the connection. So a old client talking to new daemon would never
see the async messages, unless they enabled this feature explicitly.

> Library changes
> ---------------
> 
> On receiving a status message, the library can ignore it, or can:
> 
> (1) update the total and current fields in the guestfs_h handle, and/or
> 
> (2) call a prearranged callback function.
> 
> We would add a way to query these numbers for an existing handle:
> 
>   void guestfs_get_progress (guestfs_h *g, int64_t *total, int64_t *current);
> 
> Callers can poll this function on any open handle from any thread to
> retrieve the progress of the currently running call.
> 
> [Side note: In general you cannot call libguestfs APIs from multiple
> threads:
> http://libguestfs.org/guestfs.3.html#multiple_handles_and_multiple_threads]

In the wire protocol, when sending back the status messages, it is
probably wise to make sure the 'serial' field of the status message
always matches the 'serial' field of the initial RPC request. This
means if you do make the library multi-threaded in the future, you
have the ability to correlate async status messages to the correct
thread's RPC call.

> Daemon changes
> --------------
> 
> Long running calls tend to be of two forms:
> 
>   while (n < size) {
>     copy_a_buffer ();
>     n += size_copied;
>   }
> 
> or:
> 
>   command ("long_running_external_command");
> 
> The first form can be changed easily:
> 
>   while (n < size) {
>     copy_a_buffer ();
>     n += size_copied;
>     notify_progress (n, size);
>   }
> 
> The 'notify_progress' function can be called as often as needed, and
> it would have to contain its own rate limiting functionality so that
> it doesn't actually send progress messages back more often than
> desired (eg. once per second).  This allows for a very simple change
> to all the potentially long-running daemon functions of the first
> form.
> 
> The second form are more difficult to change.  With each one we would
> have to consider the nature of the external command, if it provides
> some sort of progress feature or if we need to poll it from the daemon
> (eg. polling the size of the input and output files).
> 
> Some functions of the second form could be changed.  eg. The
> implementation of guestfs_dd runs the external "dd" command, but could
> be modified to an internal copy function of the first form, and this
> might have other benefits too.
> 
> Other functions of the second form would be changed to poll status
> over time.  There is nothing in the library API which requires any
> function to provide status messages.
> 
> Tools
> -----
> 
> Tools, such as guestfish and virt-resize, must also be changed to poll
> for status or set a callback for some long-running operations.
> Obviously setting a callback would be preferred.
> 
> We would need to change the Perl and OCaml bindings at least to
> support the progress callback.
> 
> Generator
> ---------
> 
> After making these changes we'd have a few API calls which generate
> status messages, and many calls which don't.  You can call
> guestfs_get_progress on any call, but it wouldn't return any useful
> information for the majority of calls.

If you're not very careful adding a 'guestfs_get_progress' API
could make it very hard to introduce multithread support. Since
the main thread is blocked on the original API, I assume this
get_progress will be called from a separate thread, so you can't
just use thread locals to associate this info. 

I can see two options to make this extensible to allow potential
for future concurrent usage

 - Associate a 'status' object with a thread. 

     guestfs_status_h s = guestfs_status_new();
     guestfs_set_status_object(g, s);
     guestfs_do_some_api_call(g);

   This would store 's' in a thread-local. Any API calls
   made in this thread would use that object.

   Another thread can query that same object. Thus many
   threads can setup their own status objects and each
   can be monitored independantly.

 - Add variants of each API which take a status object

     guestfs_status_h s = guestfs_status_new();
     guestfs_do_some_api_call_with_status(g, s);

   The status object is associated explicitly with an API call, so again
   concurrently API calls from different threads can be monitored
   independantly.

Regards,
Daniel
-- 
|: Red Hat, Engineering, London    -o-   http://people.redhat.com/berrange/ :|
|: http://libvirt.org -o- http://virt-manager.org -o- http://deltacloud.org :|
|: http://autobuild.org        -o-         http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505  -o-   F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]