On 09/17/2012 02:04 PM, Daniel P. Berrange wrote: > On Mon, Sep 17, 2012 at 05:48:45AM -0400, Laine Stump wrote: >> This patchset implements a new API function called virNetworkUpdate >> which enables updating certain parts of a libvirt network's definition >> without the need to destroy/re-start the network. This is especially >> useful, for example, to add/remove hosts from the dhcp static hosts >> table, or change portgroup settings. >> >> This was previously discussed in this thread: >> >> https://www.redhat.com/archives/libvir-list/2012-August/msg01535.html >> >> continuing here in September: >> >> https://www.redhat.com/archives/libvir-list/2012-September/msg00328.html >> >> with the final form here: >> >> https://www.redhat.com/archives/libvir-list/2012-September/msg00465.html >> >> In short, the single function has a "section" specifier which tells >> the part of the network definition to be updated, a "parentIndex" that >> gives the index of the *parent* element containing this section (when >> there are multiples - in particular in the case of the <ip> element), >> and a fully formed XML element which will be added as-is in the case >> of VIR_NETWORK_UPDATE_ADD_* (after checking for a duplicate), used to >> search for the specific element to delete in case of >> VIR_NETWORK_UPDATE_DELETE, and used both to find the existing element >> and replace its current contents in the case of VIR_UPDATE_EXISTING >> (this implies that you can't change the change the attribute used for >> indexing, e.g. the name of a portgroup, or mac address of a dhcp host >> entry). >> >> An example of use: to add a dhcp host entry to network "net", you would do this: >> >> virNetworkUpdate(net, VIR_NETWORK_SECTION_IP_DHCP_HOST, -1, >> "<host mac='00:11:22:33:44:55' ip='192.168.122.5'/>", >> VIR_NETWORK_UPDATE_AFFECT_LIVE >> | VIR_NETWORK_UPDATE_AFFECT_CONFIG >> | VIR_NETWORK_UPDATE_ADD_LAST); >> >> To delete that same entry: >> >> virNetworkUpdate(net, VIR_NETWORK_SECTION_IP_DHCP_HOST, -1, >> "<host mac='00:11:22:33:44:55'/>", >> VIR_NETWORK_UPDATE_AFFECT_LIVE >> | VIR_NETWORK_UPDATE_AFFECT_CONFIG >> | VIR_NETWORK_UPDATE_DELETE); >> >> If you wanted to force any of these to affect the dhcp host list in >> the 3rd <ip> element of the network, you would replace "-1" with "2". >> >> Another example: to modify the portgroup named "engineering" (e.g. to >> increase the inbound average bandwidth from 1000 to 2000): >> >> virNetworkUpdate(net, VIR_NETWORK_SECTION_PORTGROUP, -1, >> "<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_EXISTING | VIR_NETWORK_UPDATE_LIVE >> | VIR_NETWORK_UPDATE_CONFIG) >> >> (note that parentIndex is irrelevant for PORTGROUP, since they are in >> the toplevel of <network>, so there aren't multiple instances of >> parents. In such cases, the caller *must* set parentIndex to -1 or 0 - >> any other value indicates that they don't understand the purpose/usage >> of parentIndex, so it must result in an error. Also note that the >> above function would fail if it couldn't find an existing portgroup >> with name='engineering' (i.e. it wouldn't automatically add a new one).) > I've been trying to think about how this might all map into the > LibvirtGObject/LibvirtGConfig APIs, and the thing I'm struggling > with is the parentIndex parameter. > > First of all, in the GConfig API I won't be exposing the virNetworkUpdate > API as it is. To be typesafe, there will be separate APIs for each possible > operation. eg > > gvir_network_add_portgroup > gvir_network_remove_portgroup > gvir_network_update_portgroup > > > Consider how the <network> schema will eventually map into objects, > > <network> == GVirConfigNetwork > <name>test1</name> > <uuid>2d39a0ba-ac4b-6097-114c-50f8bccc277c</uuid> > <forward mode='bridge'/> > <bridge name='virbr5' stp='on' delay='0' /> > <mac address='52:54:00:38:81:4D'/> > <domain name='example.com'/> > <forward mode='private'/> > <interface dev="eth20"/> == GVirConfigNetworkInterface > <interface dev="eth21"/> == GVirConfigNetworkInterface > <interface dev="eth22"/> == GVirConfigNetworkInterface > <interface dev="eth23"/> == GVirConfigNetworkInterface > <interface dev="eth24"/> == GVirConfigNetworkInterface > </forward> > <portgroup name='engineering' default='yes'> == GVirConfigNetworkPortGroup > <virtualport type='802.1Qbh'> > <parameters profileid='test'/> > </virtualport> > <bandwidth> > <inbound average='1000' peak='5000' burst='5120'/> > <outbound average='1000' peak='5000' burst='5120'/> > </bandwidth> > </portgroup> > <portgroup name='sales'> == GVirConfigNetworkPortGroup > <virtualport type='802.1Qbh'> > <parameters profileid='salestest'/> > </virtualport> > <bandwidth> > <inbound average='500' peak='2000' burst='2560'/> > <outbound average='128' peak='256' burst='256'/> > </bandwidth> > </portgroup> > <dns> == GVirConfigNetworkDNS > <txt name="example" value="example value" /> == GVirConfigNetworkDNSEntry > <srv service='name' protocol='tcp' domain='test-domain-name' target='.' port='1024' priority='10' weight='10'/> == GVirConfigNetworkDNSEntry > <host ip='192.168.122.2'> == GVirConfigNetworkDNSEntry > <hostname>myhost</hostname> > <hostname>myhostalias</hostname> > </host> > </dns> > <ip address='10.24.75.1' netmask='255.255.255.0'> == GVirConfigNetworkAddress > <dhcp> == GVirConfigNetworkDHCP > <range start='10.24.75.128' end='10.24.75.254' /> > <host mac='52:54:3e:77:e2:ed' name='X.example.com' ip='10.24.75.10' /> == GVirConfigNetworkDHCPHost > <host mac='52:54:3e:77:e2:ef' name='Y.example.com' ip='10.24.75.11' /> > <host mac='52:54:34:77:e2:f0' name='Z.example.com' ip='10.24.75.12' /> > <host mac='52:54:3e:77:e2:f1' name='A.example.com' ip='10.24.75.13' /> > </dhcp> > </ip> > <ip address='192.168.4.1' netmask='255.255.255.0'/> == GVirConfigNetworkAddress > </network> > > > So for example we get the config object using > > GVirNetwork *net = gvir_connection_get_network_by_name("default"); > GVirConfigNetwork *conf = gvir_network_get_config(net); > > > Now we want to remove all portgroups. This is easy enough - I'd have > an API like > > GList *groups = gvir_config_network_get_portgroups(conf); > > while (groups) { > GVirConfigNetworkPortgroup *group = groups->data; > > gvir_network_remove_portgroup(net, group); > > data = data->next; > } > > > As you say, the concept of parentIndex doesn't make sense for portgroups, > so I'll just ignore it in the API I expose. > > > Likewise adding/removing <interface> from <forward>, we just ignore the > parentIndex. Modify doesn't make sense for this part of the schema > since there are no attributes to change, beyond the interface name. > > DNS entries are reasonably easy to deal with add/remove, since again > parentIndex is irrelevant, there only being one <dns> block. > > I'm a little fuzzy on whether a modify action is practical for DNS > entries, since the thing you'd want to change is probably also the > thing the API would want to use as the unique identifier. The only > way around this I see is to pass in both the original and new XML > for the DNS entry being modified. The original XML is used to lookup > which entry is being mdified, and then replace with the new XML. > Perhaps this doesn't matter, and add+remove is sufficient for DNS. > > > The updating of DHCP entries is the interesting / hard one that causes > the fun with parentIndex. > > It is possible to come up with a mapping to GObject, > > GList *addrs = gvir_config_network_get_addresses(conf); What if the private data for each address in this list had an index stored in it by gvir_config_network_get_addresses()... > int idx = 0; > > while (addrs) { > GVirConfigNetworkAddress *addr = addrs->data; > > GVirConfigNetworkDHCP *dhcp = gvir_config_network_address_get_dhcp(addr); ...and the private data for dhcp had the same index set (by gvir_config_network_address_get_dhcp() grabbing it from the addr's private data)... > GList *hosts = gvir_config_network_dhcp_get_hosts(dhcp); ...and finally, the private data of each host would have an idx that is initialized from the dhcp's private data during gvir_config_network_dhcp_get_hosts(). > while (hosts) { > GVirConfigNeworkDHCPHost *host = hosts->data; > > gvir_network_remove_dhcp_host(net, idx, host); So now when you get to here, gvir_network_remove_dhcp_host only needs (net, host), because host->idx (which is private) is already set. > } > > idx++; > data = data->next; > } > > What I don't like is that the user has to maintain this 'idx' counter > value. It doesn't hurt in this example, but consider if you were just > passed a single "GVirConfigNetworkAddress" object, and wanted to add a > host entry to it. You have no idea what the parentIndex this corresponds > to. I think storing the ip index in the private data and passing it down the hierarchy would work (until someone inserted a new <ip> element somewhere in the middle :-/) > This isn't fatal, but it is slightly unpleasant. I don't have any > better idea though, so I guess we'll just go with what you designed. Sigh. To paraphrase Hannibal Smith "I *hate* it when a plan doesn't come together!" -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list