Re: [libvirt] RFC: take two on live update of network configuration

On 09/06/2012 04:52 PM, Eric Blake wrote: > On 09/06/2012 07:43 AM, Daniel P. Berrange wrote: > >>> =========== >>> OPTION 3) Again, a single API, but with an added "nodespec" arg which would >>> be used to describe the parent node you of the node you want added/updated to: >>> >>> 3) Can you think of a situation where this wouldn't work? >> In a libvirt API where you don't use XML directly. >> >> >> The thing I like about option 3 is that you are just passing in little >> snippets of XML to either add/remove/modify. But we need to figure out >> a better way todo this, without using XPath markers IMHO. > Option 4 > > Instead of an arbitrary XPath marker, can we come up with an enum of > possible change sites? That would mean the user doesn't have quite as > much flexibility (passing an int instead of string), but we are still > extensible as new changeable sites make sense (add another enum > constant). There's still some disambiguation needed, but I suppose you > could obtain that by passing (a subset of) the original XML sufficient > to provide the context necessary. Yeah, this is more or less what we discussed separately on Wednesday, and seems to fit with both Dan and DV's ideas. > > For example, > > /* full text of <host> element to add to <dhcp>, embedded in > * enough partial context from the original xml as to find the > * right <dhcp> to add it to */ > virNetworkUpdate(VIR_NETWORK_UPDATE_DHCP, > "<network>" > " <ip address='10.24.75.1'>" > " <dhcp>" > " <host mac='00:16:3e:77:e2:ed' " > " name='X.example.com' ip='10.24.75.10'/>" > " </dhcp>" > " </ip>" > "</network>", I wouldn't include the whole hierarchy, though - just the <host> element. (instead of specifying adress='10.24.75.1' to figure out which ip address, we could use DV's suggestion of including a simple "index" argument). This may be slightly less flexible, but removes the need to deal with an XML hierarchy, and works for current needs. So the new API would be this: int virNetworkUpdate(virNetworkPtr network, unsigned int section, size_t index, char *xml, unsigned int flags); section would be one of the following: typedef enum virNetworkUpdateSection { VIR_NETWORK_SECTION_BRIDGE = 0, VIR_NETWORK_SECTION_DOMAIN = 1, VIR_NETWORK_SECTION_IP = 2, VIR_NETWORK_SECTION_IP_DHCP_HOST = 3, VIR_NETWORK_SECTION_IP_DHCP_RANGE = 4, VIR_NETWORK_SECTION_FORWARD = 5, VIR_NETWORK_SECTION_FORWARD_INTERFACE = 6, VIR_NETWORK_SECTION_FORWARD_PF = 7, VIR_NETWORK_SECTION_PORTGROUP = 8, VIR_NETWORK_SECTION_DNS_HOST = 9, VIR_NETWORK_SECTION_DNS_TXT = 10, VIR_NETWORK_SECTION_DNS_SRV = 11, }; In each case, the xml would need to be a full element named by the final bit of the enum (so a bit of redundancy, but I think necessary for consistency), and generally (with the exception of portgroup and dns host) would not have any subelements (this both makes it simpler to understand/translate into libvirt-glib, and limits the impact); for example: virNetworkUpdate(net, VIR_NETWORK_SECTION_BRIDGE, 0 "<bridge name='br0' stp='off'/>", VIR_NETWORK_UPDATE_LIVE|VIR_NETWORK_UPDATE_CONFIG); (this would modify the existing <bridge> element (presumably to turn off stp); since UPDATE_ADD wasn't specified, if there was no bridge element already present, this would fail). Note the unused "index" of 0. virNetworkUpdate(net, VIR_NETWORK_SECTION_IP_DHCP_HOST, 0, "<host mac='00:16:3e:77:e2:ed' " "name='X.example.com' ip='10.24.75.10'/>" VIR_NETWORK_UPDATE_ADD | VIR_NETWORK_UPDATE_LIVE | VIR_NETWORK_UPDATE_CONFIG); (adds a new host. If any of the attributes match an existing host, it would fail). The index is still 0 - it could be changed to "1" if we wanted to change a host entry in the dhcp of the network's 2nd ip address. (Should index instead be a const char *, allowing for character indexes, like "10.24.75.1"? Currently it's only there to allow for future expansion; maybe a single integer doesn't allow for enough expansion, but a char* would...) (or should I drop the whole idea of the index, and just include it in a separate xml element at the beginning of the xml arg when it becomes necessary?). It's important to notice that the index isn't within the dhcp's host array, but within the parent IPs. This does point out a possible deficiency with the idea of having a single index - in the future there may be a case where multiple indexes are required. Deleting a host would be done like this: virNetworkUpdate(net, VIR_NETWORK_SECTION_IP_DHCP_HOST, 0, /* just enough to match existing entry */ "<host mac='00:16:3e:77:e2:ed'/>", VIR_NETWORK_UPDATE_DELETE | VIR_NETWORK_UPDATE_LIVE | VIR_NETWORK_UPDATE_CONFIG); or if you preferred: virNetworkUpdate(net, VIR_NETWORK_SECTION_IP_DHCP_HOST, 0, /* again, just enough to match */ "<host ip='10.24.75.10'/>", VIR_NETWORK_UPDATE_DELETE |VIR_NETWORK_UPDATE_LIVE | VIR_NETWORK_UPDATE_CONFIG); To remove an interface from the interface pool: virNetworkUpdate(net, VIR_NETWORK_SECTION_FORWARD_INTERFACE, 0, "<interface dev='eth20'/>", VIR_NETWORK_UPDATE_DELETE | VIR_NETWORK_UPDATE_LIVE | VIR_NETWORK_UPDATE_CONFIG) To modify the portgroup named "engineering" (e.g. to increase the inbound average bandwidth from 1000 to 2000): virNetworkUpdate(net, VIR_NETWORK_SECTION_PORTGROUP, 0, "<portgroup name='engineering' default='yes'>" " <virtualport type='802.1Qbh'>" " <parameters profileid='test'/>" " </virtualport>" " <bandwidth>" " <inbound average='2000' peak='5000' burst='5120'/>" " <outbound average='1000' peak='5000' burst='5120'/>" " </bandwidth>" "</portgroup>", VIR_NETWORK_UPDATE_DELETE | VIR_NETWORK_UPDATE_LIVE | VIR_NETWORK_UPDATE_CONFIG) So how does this sound? Am I getting closer? Or further away?