Libguestfs - Mar 2019 - nbdkit design: varying the rate limit parameter

Hi Eric,

We may have our first user (Tomas, CC'd) of the rate filter to limit
NBD connections.  As you recall it lets you do commands like:

  nbdkit --filter=rate memory size=64G rate=10M

to limit network bandwidth to 10 Mbps.  However a twist is that he
needs to vary the parameter while nbdkit is running in response to a
UI, and we haven't really thought about that before.

In the grand and noble tradition of qemu we don't actually systematize
parameters.  For the server they are just (key, value) strings.  (It's
even possible for filters to modify plugin parameters, thankfully
nothing does that.)

The only filter we have which does anything similar is the error
filter, where the error-file parameter lets you enable and disable the
filter at runtime based on the presence of a file.

  rm -f /tmp/inject
  nbdkit --filter=error file disk.img error-rate=100%  error-file=/tmp/inject
  sleep 60; touch /tmp/inject

I think our options are:

* One-time hack in the rate filter.

* One-time hack in the rate filter, but make it compatible with a
  possible future revision of parameter handling.  eg: If we decided
  that "rate:file" would in future mean "read ‘rate’ parameter
from a
  file" then we could add this as a simple parameter now but
  systematize it in future.

* Systematize parameters.  This would require a great deal of work and
  likely another version of the API.  (I pushed a note into the TODO
  file for this.)

If we went all the way and allowed parameter values to change, would
this be triggered only by simple file changes, or would we want an
administration interface to the server?  (a la QMP)

But I wonder if you have any better ideas?

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-df lists disk usage of guests without needing to install any
software inside the virtual machine.  Supports Linux and Windows.
http://people.redhat.com/~rjones/virt-df/

On 3/26/19 11:13 AM, Richard W.M. Jones wrote:
> Hi Eric,
> 
> We may have our first user (Tomas, CC'd) of the rate filter to limit
> NBD connections.  As you recall it lets you do commands like:
> 
>   nbdkit --filter=rate memory size=64G rate=10M
> 
> to limit network bandwidth to 10 Mbps.  However a twist is that he
> needs to vary the parameter while nbdkit is running in response to a
> UI, and we haven't really thought about that before.
> 
> In the grand and noble tradition of qemu we don't actually systematize
> parameters.  For the server they are just (key, value) strings.  (It's
> even possible for filters to modify plugin parameters, thankfully
> nothing does that.)
> 
> The only filter we have which does anything similar is the error
> filter, where the error-file parameter lets you enable and disable the
> filter at runtime based on the presence of a file.
> 
>   rm -f /tmp/inject
>   nbdkit --filter=error file disk.img error-rate=100% 
error-file=/tmp/inject
>   sleep 60; touch /tmp/inject
And interesting that it is the presence (and not the contents) of that file.
> 
> I think our options are:
> 
> * One-time hack in the rate filter.
> 
> * One-time hack in the rate filter, but make it compatible with a
>   possible future revision of parameter handling.  eg: If we decided
>   that "rate:file" would in future mean "read ‘rate’
parameter from a
>   file" then we could add this as a simple parameter now but
>   systematize it in future.
So if I understand the proposal, we'd use "name:type" (where :type
is
optional and defaults to :string), so that in the future we can define
other valid types (:file meaning to read from a file, and pay attention
to whether that file changes at runtime).  It would also mean we could
add types such as :int or :size to get nbdkit to do sanity checking that
a string is indeed parseable as a particular type of integer, or :bool
to automatically use nbdkit_parse_bool, rather than having to open-code
those checks in each plugin/filter using a parameter with those types.
> 
> * Systematize parameters.  This would require a great deal of work and
>   likely another version of the API.  (I pushed a note into the TODO
>   file for this.)
Have we documented any restrictions on what forms a valid parameter
name? If we don't allow ':' in current parameter names, that's
nicer
(because we can add the extension of 'name:type') than if our new
interpretation of ':' could interfere with existing plugin's
parameter
names.  Then again, abusing 'name:type' instead of having an actual
struct may be a reason why a v3 protocol that uses an actual struct
could be easier to work with.
> If we went all the way and allowed parameter values to change, would
> this be triggered only by simple file changes, or would we want an
> administration interface to the server?  (a la QMP)
It sounds like a lot of work to get an administration interface,
compared to just stating that a particular dynamic value will be
obtained by re-reading a given file name each time the parameter's value
is needed at runtime.
> 
> But I wonder if you have any better ideas?
> 
> Rich.
> 
-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

On Thu, Mar 28, 2019 at 11:45:06AM -0500, Eric Blake
wrote:
> On 3/26/19 11:13 AM, Richard W.M. Jones wrote:
> > I think our options are:
> > 
> > * One-time hack in the rate filter.
> > 
> > * One-time hack in the rate filter, but make it compatible with a
> >   possible future revision of parameter handling.  eg: If we decided
> >   that "rate:file" would in future mean "read ‘rate’
parameter from a
> >   file" then we could add this as a simple parameter now but
> >   systematize it in future.
> 
> So if I understand the proposal, we'd use "name:type" (where
:type is
> optional and defaults to :string), so that in the future we can define
> other valid types (:file meaning to read from a file, and pay attention
> to whether that file changes at runtime).  It would also mean we could
> add types such as :int or :size to get nbdkit to do sanity checking that
> a string is indeed parseable as a particular type of integer, or :bool
> to automatically use nbdkit_parse_bool, rather than having to open-code
> those checks in each plugin/filter using a parameter with those types.
I didn't necessarily mean it would have to be that exact scheme, I
meant any scheme as long as it could plausibly be implemented properly
in future while preserving back-compat.  If you can think of something
better ...
> > * Systematize parameters.  This would require a great deal of work and
> >   likely another version of the API.  (I pushed a note into the TODO
> >   file for this.)
> 
> Have we documented any restrictions on what forms a valid parameter
> name? If we don't allow ':' in current parameter names,
that's nicer
> (because we can add the extension of 'name:type') than if our new
> interpretation of ':' could interfere with existing plugin's
parameter
> names.  Then again, abusing 'name:type' instead of having an actual
> struct may be a reason why a v3 protocol that uses an actual struct
> could be easier to work with.
The code only allows [-A-Za-z0-9._]+ (server/main.c:is_config_key) and
this is also documented in the manual.  So ':' is currently not
permitted in keys.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-p2v converts physical machines to virtual machines.  Boot with a
live CD or over the network (PXE) and turn machines into KVM guests.
http://libguestfs.org/virt-v2v

I'm embellishing this quite a lot, but this was an alternative
proposal we discussed on IRC, originated by Pino Toscano:

We'd add a "reload" callback to plugins and filters.  Calling this
is
triggered when the server receives SIGHUP [currently ignored by the
server].  The plugins and filters could treat this any way they
wanted, but a suggestion would be that if they are taking
configuration from a file then they would reload the information from
that file.

Note that nbdkit itself does not have a configuration file, and we
don't really plan one, so this doesn't affect the server.  Only the
error filter uses an external "configuration file" (of sorts) but
that's a special case so I don't think it would use this file.

We'd then implement the rate filter with new parameters rate-file and
connection-rate-file which would read the rate out of a file, and wire
up the reload callback to re-read those files.

I think we'd likely want the reload callback to serialise with all
other callbacks, even for fully parallel plugins.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-df lists disk usage of guests without needing to install any
software inside the virtual machine.  Supports Linux and Windows.
http://people.redhat.com/~rjones/virt-df/