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@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list