On 30.11.2012 19:43, Laine Stump wrote:
> On 11/19/2012 11:51 AM, Michal Privoznik wrote:
>> Network should be notified if we plug in or unplug an
>> interface, so it can perform some action, e.g. set/unset
>> network part of QoS.
>> ---
>> src/Makefile.am | 7 ++-
>> src/conf/domain_conf.h | 1 +
>> src/conf/network_conf.c | 1 +
>> src/conf/network_conf.h | 3 +
>> src/libvirt_network.syms | 8 ++
>> src/network/bridge_driver.c | 165 +++++++++++++++++++++++++++++++++++++++++++
>> src/network/bridge_driver.h | 10 ++-
>> src/qemu/qemu_command.c | 32 ++++++---
>> src/qemu/qemu_process.c | 12 +++
>> 9 files changed, 225 insertions(+), 14 deletions(-)
>> create mode 100644 src/libvirt_network.syms
>>
>> diff --git a/src/conf/network_conf.h b/src/conf/network_conf.h
>> index 3e46304..8a8d1e4 100644
>> --- a/src/conf/network_conf.h
>> +++ b/src/conf/network_conf.h
>> @@ -221,6 +221,9 @@ struct _virNetworkObj {
>>
>> virNetworkDefPtr def; /* The current definition */
>> virNetworkDefPtr newDef; /* New definition to activate at shutdown */
>> +
>> + unsigned int class_id; /* next class ID for QoS */
>
> Okay, so this is just a monotonically increasing counter so that each
> interface gets a new and different class_id. Maybe you should call it
> "nextFreeClassID" or something like that, so that everyone understands
> it's not a class id used by the network. Or you might want to do this
> with a bitmap so you can re-use id's of interfaces that are
> disconnected. (can virBitmaps being dynamically expanded?)
>
>> + unsigned long long floor_sum; /* sum of all 'floor'-s of attached NICs */
>
> And you keep track of the sum of all floors here.
>
> So, what happens if libvirtd is restarted? It looks like they both go
> back to their initial values.
We think alike :) In the next patches I am fixing this. class_id becomes
a virBitmap and among with floor_sum gets stored into network status
XML. I've split it into several patches because changes are big (in
number of lines at least) and I think this way it's easier to review.
>
> And what about hotplug?
>
> This is a similar problem to the pool of interfaces used by
> macvtap/hostdev modes - a network with an interface pool needs to have
> the "inuse" counters of each interface refreshed whenever libvirtd
> restarts. So the necessary hooks are already in place:
>
> networkAllocateActualDevice - called any time an interface with
> type='network' is initially
> allocated and connected.
>
> networkNotifyActualDevice - called for each type='network' interface
> of each active domain
> any time libvirtd restarts
>
> networkReleaseActualDevice - called for every type='network'
> interface as it is being
> disconnected and destroyed.
>
> These are called for *all* type='network' interfaces, not just those
> that need to allocate a physical netdev from a pool.
>
> Rather than adding your own new hooks (and figuring out all the places
> they need to be called), I think it would be better to use the existing
> hooks (perhaps calling a reduced/renamed version of your functions,
> which can possibly be moved over to ). That will have two advantages:
>
> 1) It will assure that the bandwidth functions are called at all the
> right times, including hotplug/unplug, and libvirtd restart
>
> 2) It will continue the process of consolidating all network-related
> functionality need for these three events into 3 functions which may
> some day be moved into their own daemom with a public (within libvirt
> anyway) API accessible via RPC (thus allowing non-privileged libvirts to
> have full networking functionality).
>
Okay, renamed to networkPlugBandwidth and networkUnplugBandwidth and
inserted into networkAllocateActualDevice and networkReleaseActualDevice
respectively.
> Note that you will probably want to save the interface class_id in the
> iface->actual (as a matter of fact, in
> iface->data.network.actual->bandwidth, which can be retrieved with
> virDomainNetGetActualBandwidth()) so that it's saved properly in the
> interface's state without polluting the interface's config. Then it will
> be read back from the state file during the restart and available during
> networkNotifyActualDevice() of each interface. I guess you can then
> re-set the network->class_id by checking interface->class_id and setting
>
> network->class_id = MAX(network->class_id, iface->class_id + 1);
>
> for each encountered interface (or add it to a bitmap, if a) bitmaps can
> be enlarged dynamically or b) we can determine a reasonable maximum
> limit on number of active domains). At the same time, you'll recompute
> the network->floor_sum iteratively as the network is called with a
> notify for each interface.
>
>> ...
>
> To summarize: I think this needs to be refactored to be integrated into
> the network*ActualDevice() functions so that the bookkeeping is properly
> handled in all cases, including hotplug and libvirtd restart.
>
> --
> libvir-list mailing list
> libvir-list@xxxxxxxxxx
> https://www.redhat.com/mailman/listinfo/libvir-list
>
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list