[libvirt] RFC: take two on live update of network configuration
We need a way to update the elements of a <network> without being required to restart the network before the changes take effect. I had previously thought I could just use a new virNetworkDefineXMLFlags with an optional flag set to mean "take effect immediately", but have been discouraged from that approach. Instead, I'm considering something similar to virDomain(Attach|Update|Detach)DeviceFlags, but this could be more complicated because I would like to be able to update *anything* within the network definition hierarchy, not just the direct subelements of one level in the hierarchy (as is the case for those existing functions). Here's an example of an existing network (note that this isn't a legal network - you can't have an <ip> element and an interface pool (or a bridge name and an interface pool) in the same network; this is just so I can use a single example), and some of the things we might want to do with it: <network> <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"/> <interface dev="eth21"/> <interface dev="eth22"/> <interface dev="eth23"/> <interface dev="eth24"/> </forward> <portgroup name='engineering' default='yes'> <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></b> <portgroup name='sales'> <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></b> <dns> <txt name="example" value="example value" /> <srv service='name' protocol='tcp' domain='test-domain-name' target='.' port='1024' priority='10' weight='10'/> <host ip='192.168.122.2'> <hostname>myhost</hostname> <hostname>myhostalias</hostname> </host> </dns> <ip address='10.24.75.1' netmask='255.255.255.0'> <dhcp> <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' /> <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'/> </network> Some things we might want to do to this (the first three have already been requested): 1) add or remove an interface from the interface pool, i.e. a particular <interface> sub-element of the (one and only) <forward> element. 2) add/remove/modify a portgroup entry (i.e. a specific sub-element of <network>) 3) add/remove/modify a static host entry from the <dhcp> section of a particular <ip> (so, a particular <host> sub-element of a particular <ip> element) Some others: 4) add/modify/remove a <host> (or a <txt> or an <srv> in the <dns> section 4) add/modify/remove the <domain> 5) add/modify/remove an entire <ip> element 6) turn stp on/off in the <bridge> element I can see three possible methods of providing this functionality: =========== OPTION 1) Have a single API: virNetworkUpdate(virNetworkPtr network, const char *xml, unsigned, int flags) This function would take an entire <network> specification as an arg, and replace the entire existing config. This would allow changing anything, but would also require reading the entire config beforehand. =========== OPTION 2) have a separate API for each different type of element we may want to change, e.g.: virNetworkUpdateForwardInterface(virNetworkPtr network, const char *xml, unsigned int flags); virNetworkUpdatePortgroup(virNetworkPtr network, const char *xml, unsigned int flags); virNetworkUpdateIpDhcpHost(virNetworkPtr network, const char *xml, unsigned int flags); virNetworkUpdateDnsEntry(virNetworkPtr network, const char *xml, unsigned int flags); /* The name of this one may confuse... */ virNetworkUpdateDomain(virNetworkPtr network, const char *xml, unsigned int flags); virNetworkUpdateBridge(virNetworkPtr network, const char *xml, unsigned int flags); virNetworkUpdateIpDnsHost(virNetworkPtr network, const char *xml, unsigned int flags); etc. (etc. etc.) "flags" would include: /* force add if object with matching key doesn't exist */ VIR_NETWORK_UPDATE_ADD /* delete existing object(s) that matches the xml */ VIR_NETWORK_UPDATE_DELETE /* take effect immediately */ VIR_NETWORK_UPDATE_LIVE /* persistent change */ VIR_NETWORK_UPDATE_CONFIG This method could be problematic, e.g., for something like virNetworkUpdateIpDhcpHost() - although currently only a single <ip> element can have a <dhcp> entries, in the future we could allow any/all of them to have dhcp entries. Another downside is that we would need to add an new function for each new element we decide we want to be able to update. =========== 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: int virNetworkUpdate(virNetworkPtr network, const char *nodeSpec, const char *xml, unsigned int flags); For example, if you wanted to add a new <host> entry to <ip address='10.24.75.1' ...>'s dhcp element, you would do this: /* nodespec obviously uses an XPath expression */ virNetworkUpdate("/ip[address='10.24.75.1']/dhcp", /* full text of <host> element to add */ "<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); Or, to change the contents of the exiting portgroup "engineering" you would use: virNetworkUpdate("/", /* full text of portgroup */, "<portgroup name='engineering'> ..... </portgroup>" VIR_NETWORK_UPDATE_LIVE|VIR_NETWORK_UPDATE_CONFIG); To delete a static dhcp host entry, you would use this: virNetworkUpdate("/ip[address='10.24.75.1']/dhcp", /* 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("/ip[address='10.24.75.1']/dhcp", /* 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("/forward", "<interface dev='eth20'/>", VIR_NETWORK_UPDATE_DELETE | VIR_NETWORK_UPDATE_LIVE | VIR_NETWORK_UPDATE_CONFIG) (adding an interface would be identical, except the flag). (An alternate implementation would be to have nodeSpec specify the node that is being updated/removed rather than its parent. This may make more logical sense, (although not for ADD) and would even make the rest of the code simpler for update/remove (we wouldn't have to do a manual search within the node). The positive side of this is that the one API function allows you to modify *anything* (even stuff not yet invented, so it's future-proof). The negative side is that the code that implements it would need to go to quite a bit of extra trouble to determine what has been changed (basically, it would a) generate XML for the current state of the network, b) parse that into an xmlNode, c) perform the operation on xmlNode using nodeset to figure out where, and adding in the new xml (or removing any nodes matching it), d) convert modified xmlNode back to xml text, e) parse the xml text into a virNetworkDef, f) compare it to the original virNetworkDef to determine what to do. ========== Which OPTION? 1, 2, or 3? ========================= I'm discounting option (1) for now. Here's the scorecard for (2) vs. (3): 2) Pros: easier to understand, more exactly defined. Cons: requires a new API each time we find a different node we want to be able to update 3) Pros: one API covers everything, should never need any new API Cons: more complex for user to understand, me to implement. Possibly more trouble to do fine grained access control (e.g. if you want to allow adding dhcp static hosts, but not allow changing the domain name or interface pool) (actually, in practice, I guess it shouldn't be any more trouble to do fine grained access control, as long as you can make the decision of whether or not to allow later on in the function that implements the API (at step (6) above), rather than at the top level when the API is first called). I'm leaning towards (3). If it raises a technical boundary, or people don't like the idea of having an arg in the public API that takes an XPath expression, then maybe I could switch to (2). Any opinions on which direction I should take? Some way different from what I'm suggesting? Specific questions: 1) Does having a single public API that is used to update many different parts of the object raise any hurdles wrt. fine grained access control? (knowing that there will be "some point" in that code that we know exactly what the user is attempting to change). 2) any problems / reservations about accepting XPatch expressions in the args of a public API? 3) Can you think of a situation where this wouldn't work?
On Tue, Aug 21, 2012 at 11:26:08PM -0400, Laine Stump wrote: [...]
=========== OPTION 2) have a separate API for each different type of element we may want to change, e.g.:
virNetworkUpdateForwardInterface(virNetworkPtr network, const char *xml, unsigned int flags); virNetworkUpdatePortgroup(virNetworkPtr network, const char *xml, unsigned int flags); virNetworkUpdateIpDhcpHost(virNetworkPtr network, const char *xml, unsigned int flags); virNetworkUpdateDnsEntry(virNetworkPtr network, const char *xml, unsigned int flags);
/* The name of this one may confuse... */ virNetworkUpdateDomain(virNetworkPtr network, const char *xml, unsigned int flags); virNetworkUpdateBridge(virNetworkPtr network, const char *xml, unsigned int flags); virNetworkUpdateIpDnsHost(virNetworkPtr network, const char *xml, unsigned int flags); etc. (etc. etc.)
"flags" would include:
/* force add if object with matching key doesn't exist */ VIR_NETWORK_UPDATE_ADD /* delete existing object(s) that matches the xml */ VIR_NETWORK_UPDATE_DELETE /* take effect immediately */ VIR_NETWORK_UPDATE_LIVE /* persistent change */ VIR_NETWORK_UPDATE_CONFIG
This method could be problematic, e.g., for something like virNetworkUpdateIpDhcpHost() - although currently only a single <ip> element can have a <dhcp> entries, in the future we could allow any/all of them to have dhcp entries. Another downside is that we would need to add an new function for each new element we decide we want to be able to update.
that's an awful lot of potential APIs something more generic is needed
=========== 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:
int virNetworkUpdate(virNetworkPtr network, const char *nodeSpec, const char *xml, unsigned int flags);
For example, if you wanted to add a new <host> entry to <ip address='10.24.75.1' ...>'s dhcp element, you would do this:
/* nodespec obviously uses an XPath expression */ virNetworkUpdate("/ip[address='10.24.75.1']/dhcp", /* full text of <host> element to add */ "<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);
Or, to change the contents of the exiting portgroup "engineering" you would use:
virNetworkUpdate("/", /* full text of portgroup */, "<portgroup name='engineering'> ..... </portgroup>" VIR_NETWORK_UPDATE_LIVE|VIR_NETWORK_UPDATE_CONFIG);
To delete a static dhcp host entry, you would use this:
virNetworkUpdate("/ip[address='10.24.75.1']/dhcp",
shouldn't that be //ip[address='10.24.75.1']/dhcp or your really expect to have ip as the root ?
/* 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("/ip[address='10.24.75.1']/dhcp", /* 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("/forward", "<interface dev='eth20'/>", VIR_NETWORK_UPDATE_DELETE | VIR_NETWORK_UPDATE_LIVE | VIR_NETWORK_UPDATE_CONFIG)
(adding an interface would be identical, except the flag).
(An alternate implementation would be to have nodeSpec specify the node that is being updated/removed rather than its parent. This may make more logical sense, (although not for ADD) and would even make the rest of the code simpler for update/remove (we wouldn't have to do a manual search within the node).
The positive side of this is that the one API function allows you to modify *anything* (even stuff not yet invented, so it's future-proof). The negative side is that the code that implements it would need to go to quite a bit of extra trouble to determine what has been changed (basically, it would
a) generate XML for the current state of the network, b) parse that into an xmlNode, c) perform the operation on xmlNode using nodeset to figure out where, and adding in the new xml (or removing any nodes matching it), d) convert modified xmlNode back to xml text, e) parse the xml text into a virNetworkDef, f) compare it to the original virNetworkDef to determine what to do.
One problem I can see with 3) is that it would work for now but if we were to add namespaces to the XML then the API would not allow to address those elements/attributes //foo in XPath cannot address an element foo with a namespace to do this you need to do //prefix:foo and provide separately the mapping between prefix and the namespace URI So that will work as long as we don't add namespace to the XML Another problem is making sure we have the addressing complete, for example suppose we have < The other problem is that people who already have a beef with XML usage will see red when they see the need to learn XPath O:-) , but that could be alleviated in a large part with a good set of examples ! Also the problem with Update APIs to XML is that the next step is people will want to add one part (not replace) and then you need to define where to add, before/after ... XML editing APIs are *hard*, I'm actually a bit worried to go there something like 2/ while potentially leading to more entry points probably makes thing easier for the user. Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/
On Wed, Aug 22, 2012 at 11:47:03AM +0800, Daniel Veillard wrote:
On Tue, Aug 21, 2012 at 11:26:08PM -0400, Laine Stump wrote: [...] [...] So that will work as long as we don't add namespace to the XML Another problem is making sure we have the addressing complete, for example suppose we have <
Oops i didn't finish that argument supose we have somewhere <foo> ... </foo> <foo> ... </foo> first are the element always sorted in a coherent fashion internally so that if we do //foo[2] we know which one is pointed at, or if we can guarantee order can we guarantee that for all constructs there is an identifier like an attribute allowing unique selection (like availability of an ip address) My concern is an API with hole, where there is no way to guarantee reaching the element we want to modify, and risking touching something else. Say for example 2 interface with DHCP and no IP... Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/
On 08/21/2012 11:55 PM, Daniel Veillard wrote:
On Wed, Aug 22, 2012 at 11:47:03AM +0800, Daniel Veillard wrote:
On Tue, Aug 21, 2012 at 11:26:08PM -0400, Laine Stump wrote: [...] [...] So that will work as long as we don't add namespace to the XML Another problem is making sure we have the addressing complete, for example suppose we have < Oops i didn't finish that argument
supose we have somewhere
<foo> ... </foo> <foo> ... </foo>
first are the element always sorted in a coherent fashion internally so that if we do //foo[2] we know which one is pointed at, or if we can guarantee order can we guarantee that for all constructs there is an identifier like an attribute allowing unique selection (like availability of an ip address) My concern is an API with hole, where there is no way to guarantee reaching the element we want to modify, and risking touching something else. Say for example 2 interface with DHCP and no IP...
That particular example could never happen, but in the general case I see what you mean.
On 08/21/2012 11:47 PM, Daniel Veillard wrote:
=========== OPTION 2) have a separate API for each different type of element we may want to change, e.g.:
virNetworkUpdateForwardInterface(virNetworkPtr network, const char *xml, unsigned int flags); virNetworkUpdatePortgroup(virNetworkPtr network, const char *xml, unsigned int flags); virNetworkUpdateIpDhcpHost(virNetworkPtr network, const char *xml, unsigned int flags); virNetworkUpdateDnsEntry(virNetworkPtr network, const char *xml, unsigned int flags);
/* The name of this one may confuse... */ virNetworkUpdateDomain(virNetworkPtr network, const char *xml, unsigned int flags); virNetworkUpdateBridge(virNetworkPtr network, const char *xml, unsigned int flags); virNetworkUpdateIpDnsHost(virNetworkPtr network, const char *xml, unsigned int flags); etc. (etc. etc.)
"flags" would include:
/* force add if object with matching key doesn't exist */ VIR_NETWORK_UPDATE_ADD /* delete existing object(s) that matches the xml */ VIR_NETWORK_UPDATE_DELETE /* take effect immediately */ VIR_NETWORK_UPDATE_LIVE /* persistent change */ VIR_NETWORK_UPDATE_CONFIG
This method could be problematic, e.g., for something like virNetworkUpdateIpDhcpHost() - although currently only a single <ip> element can have a <dhcp> entries, in the future we could allow any/all of them to have dhcp entries. Another downside is that we would need to add an new function for each new element we decide we want to be able to update.
On Tue, Aug 21, 2012 at 11:26:08PM -0400, Laine Stump wrote: [...] that's an awful lot of potential APIs something more generic is needed
=========== 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:
int virNetworkUpdate(virNetworkPtr network, const char *nodeSpec, const char *xml, unsigned int flags);
For example, if you wanted to add a new <host> entry to <ip address='10.24.75.1' ...>'s dhcp element, you would do this:
/* nodespec obviously uses an XPath expression */ virNetworkUpdate("/ip[address='10.24.75.1']/dhcp", /* full text of <host> element to add */ "<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);
Or, to change the contents of the exiting portgroup "engineering" you would use:
virNetworkUpdate("/", /* full text of portgroup */, "<portgroup name='engineering'> ..... </portgroup>" VIR_NETWORK_UPDATE_LIVE|VIR_NETWORK_UPDATE_CONFIG);
To delete a static dhcp host entry, you would use this:
virNetworkUpdate("/ip[address='10.24.75.1']/dhcp", shouldn't that be //ip[address='10.24.75.1']/dhcp or your really expect to have ip as the root ?
Keep in mind that I've only used xpath expressions as much as I've needed them in libvirt's parsing functions, which usually use "./xxxxx" style, so I probably got the syntax wrong. I had figured that the root node would be <network>, and would be "/". I *think* I've just learned from you that isn't the case, and that an <ip> element within <network> would be specified (if network was the root node) as "//ip".
/* 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("/ip[address='10.24.75.1']/dhcp", /* 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("/forward", "<interface dev='eth20'/>", VIR_NETWORK_UPDATE_DELETE | VIR_NETWORK_UPDATE_LIVE | VIR_NETWORK_UPDATE_CONFIG)
(adding an interface would be identical, except the flag).
(An alternate implementation would be to have nodeSpec specify the node that is being updated/removed rather than its parent. This may make more logical sense, (although not for ADD) and would even make the rest of the code simpler for update/remove (we wouldn't have to do a manual search within the node).
The positive side of this is that the one API function allows you to modify *anything* (even stuff not yet invented, so it's future-proof). The negative side is that the code that implements it would need to go to quite a bit of extra trouble to determine what has been changed (basically, it would
a) generate XML for the current state of the network, b) parse that into an xmlNode, c) perform the operation on xmlNode using nodeset to figure out where, and adding in the new xml (or removing any nodes matching it), d) convert modified xmlNode back to xml text, e) parse the xml text into a virNetworkDef, f) compare it to the original virNetworkDef to determine what to do.
One problem I can see with 3) is that it would work for now but if we were to add namespaces to the XML then the API would not allow to address those elements/attributes
//foo in XPath cannot address an element foo with a namespace
to do this you need to do
//prefix:foo and provide separately the mapping between prefix and the namespace URI
So that will work as long as we don't add namespace to the XML
Hmm. I had been thinking recently of adding a private namespace to allow adding custom commandline options to the dnsmasq commandline similar to how we currently use a private namespace to allow adding custom commandline options to the qemu commandline. Maybe something like this: network xmlns:dnsmasq='http://libvirt.org/schemas/network/dnsmasq/1.0'> <ip address='192.168.122.1' netmask='255.255.255.0'> <dhcp> <range start='10.24.75.128' end='10.24.75.254' /> <dnsmasq:commandline> <dnsmasq:arg value='--max-ttl'/> <dnsmasq:arg value='25400'/> <dnsmasq:arg value='--resolv-file=/etc/libvirt-resolv.conf'/> </dnsmasq:commandline> </dhcp> </ip> </network> So you're saying that xpath wouldn't allow me to select the <dnsmasq:commandline> element and replace it? (my guess at the expression to do that would have been "//ip[address='10.24.75.128']/dhcp/dnsmasq::commandline")
Another problem is making sure we have the addressing complete, for example suppose we have <
The other problem is that people who already have a beef with XML usage will see red when they see the need to learn XPath O:-) , but that could be alleviated in a large part with a good set of examples !
Also the problem with Update APIs to XML is that the next step is people will want to add one part (not replace) and then you need to define where to add, before/after ...
Fortunately there is nothing in <network> that is order-dependent, but in the general sense I understand the problem you're describing. I think what I'm taking away from your comments is that this isn't the 100% future-proof solution that I'd imagined.
XML editing APIs are *hard*, I'm actually a bit worried to go there something like 2/ while potentially leading to more entry points probably makes thing easier for the user.
Or maybe I should reconsider option (1) (a single API that just replaces the entire XML for the network). That would also require special considerations for fine-grained access control though - it would need to compare the old and new, then check to see if the current user was authorized to change the bits that showed up in the diff. Again, I don't know how well (if at all) that fits in with the model danpb is planning to use for that - if it's a model where user's are granted access purely on a per-API basis, then it won't work. If it's possible to do the finer-grained check further down in the implementation of the API, then it might work.
On Tue, Aug 21, 2012 at 11:26:08PM -0400, Laine Stump wrote:
We need a way to update the elements of a <network> without being required to restart the network before the changes take effect. I had previously thought I could just use a new virNetworkDefineXMLFlags with an optional flag set to mean "take effect immediately", but have been discouraged from that approach.
Instead, I'm considering something similar to virDomain(Attach|Update|Detach)DeviceFlags, but this could be more complicated because I would like to be able to update *anything* within the network definition hierarchy, not just the direct subelements of one level in the hierarchy (as is the case for those existing functions).
Here's an example of an existing network (note that this isn't a legal network - you can't have an <ip> element and an interface pool (or a bridge name and an interface pool) in the same network; this is just so I can use a single example), and some of the things we might want to do with it:
<network> <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"/> <interface dev="eth21"/> <interface dev="eth22"/> <interface dev="eth23"/> <interface dev="eth24"/> </forward> <portgroup name='engineering' default='yes'> <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></b> <portgroup name='sales'> <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></b> <dns> <txt name="example" value="example value" /> <srv service='name' protocol='tcp' domain='test-domain-name' target='.' port='1024' priority='10' weight='10'/> <host ip='192.168.122.2'> <hostname>myhost</hostname> <hostname>myhostalias</hostname> </host> </dns> <ip address='10.24.75.1' netmask='255.255.255.0'> <dhcp> <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' /> <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'/> </network>
Some things we might want to do to this (the first three have already been requested):
1) add or remove an interface from the interface pool, i.e. a particular <interface> sub-element of the (one and only) <forward> element.
2) add/remove/modify a portgroup entry (i.e. a specific sub-element of <network>)
3) add/remove/modify a static host entry from the <dhcp> section of a particular <ip> (so, a particular <host> sub-element of a particular <ip> element)
Some others:
4) add/modify/remove a <host> (or a <txt> or an <srv> in the <dns> section 4) add/modify/remove the <domain> 5) add/modify/remove an entire <ip> element 6) turn stp on/off in the <bridge> element
I can see three possible methods of providing this functionality:
=========== OPTION 1) Have a single API:
virNetworkUpdate(virNetworkPtr network, const char *xml, unsigned, int flags)
This function would take an entire <network> specification as an arg, and replace the entire existing config. This would allow changing anything, but would also require reading the entire config beforehand.
Not only that, it requires that you basically write a "diff" command for the entire network XML. eg you need to compare both the new and old config to figure out what's added, what's removed, and what's changed. Further from that, you then need to decide which changes are legal and which aren't. Then when you come to actually performing the changes, you might get 1/2 way through some changes and fail. Rolling back what you have already done is not practical, since rollback could fail too. Carrying on its obvious not reasonable, since we need to report an error about what went wrong. So now we have to merge the bits we did change with the existing config - you can't simply replace the entire old config with the new one. IMHO this is pretty non-trivial and not a route I think we should go down.
=========== OPTION 2) have a separate API for each different type of element we may want to change, e.g.:
virNetworkUpdateForwardInterface(virNetworkPtr network, const char *xml, unsigned int flags); virNetworkUpdatePortgroup(virNetworkPtr network, const char *xml, unsigned int flags); virNetworkUpdateIpDhcpHost(virNetworkPtr network, const char *xml, unsigned int flags); virNetworkUpdateDnsEntry(virNetworkPtr network, const char *xml, unsigned int flags);
/* The name of this one may confuse... */ virNetworkUpdateDomain(virNetworkPtr network, const char *xml, unsigned int flags); virNetworkUpdateBridge(virNetworkPtr network, const char *xml, unsigned int flags); virNetworkUpdateIpDnsHost(virNetworkPtr network, const char *xml, unsigned int flags); etc. (etc. etc.)
"flags" would include:
/* force add if object with matching key doesn't exist */ VIR_NETWORK_UPDATE_ADD /* delete existing object(s) that matches the xml */ VIR_NETWORK_UPDATE_DELETE /* take effect immediately */ VIR_NETWORK_UPDATE_LIVE /* persistent change */ VIR_NETWORK_UPDATE_CONFIG
This method could be problematic, e.g., for something like virNetworkUpdateIpDhcpHost() - although currently only a single <ip> element can have a <dhcp> entries, in the future we could allow any/all of them to have dhcp entries. Another downside is that we would need to add an new function for each new element we decide we want to be able to update.
Yep, this is not entirely satisfactory in terms of number of APIs.
=========== 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:
int virNetworkUpdate(virNetworkPtr network, const char *nodeSpec, const char *xml, unsigned int flags);
For example, if you wanted to add a new <host> entry to <ip address='10.24.75.1' ...>'s dhcp element, you would do this:
/* nodespec obviously uses an XPath expression */ virNetworkUpdate("/ip[address='10.24.75.1']/dhcp",
I can't say I'm really a fan of including XPath as part of the API. I don't think this kind of API would map very nicely into alternative API models such as libvirt-gobject where you don't operate in terms of XML at all.
/* full text of <host> element to add */ "<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);
Or, to change the contents of the exiting portgroup "engineering" you would use:
virNetworkUpdate("/", /* full text of portgroup */, "<portgroup name='engineering'> ..... </portgroup>" VIR_NETWORK_UPDATE_LIVE|VIR_NETWORK_UPDATE_CONFIG);
To delete a static dhcp host entry, you would use this:
virNetworkUpdate("/ip[address='10.24.75.1']/dhcp", /* 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("/ip[address='10.24.75.1']/dhcp", /* 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("/forward", "<interface dev='eth20'/>", VIR_NETWORK_UPDATE_DELETE | VIR_NETWORK_UPDATE_LIVE | VIR_NETWORK_UPDATE_CONFIG)
(adding an interface would be identical, except the flag).
(An alternate implementation would be to have nodeSpec specify the node that is being updated/removed rather than its parent. This may make more logical sense, (although not for ADD) and would even make the rest of the code simpler for update/remove (we wouldn't have to do a manual search within the node).
The positive side of this is that the one API function allows you to modify *anything* (even stuff not yet invented, so it's future-proof). The negative side is that the code that implements it would need to go to quite a bit of extra trouble to determine what has been changed (basically, it would
a) generate XML for the current state of the network, b) parse that into an xmlNode, c) perform the operation on xmlNode using nodeset to figure out where, and adding in the new xml (or removing any nodes matching it), d) convert modified xmlNode back to xml text, e) parse the xml text into a virNetworkDef, f) compare it to the original virNetworkDef to determine what to do.
==========
Which OPTION? 1, 2, or 3? =========================
I'm discounting option (1) for now. Here's the scorecard for (2) vs. (3):
2) Pros: easier to understand, more exactly defined. Cons: requires a new API each time we find a different node we want to be able to update
3) Pros: one API covers everything, should never need any new API Cons: more complex for user to understand, me to implement. Possibly more trouble to do fine grained access control (e.g. if you want to allow adding dhcp static hosts, but not allow changing the domain name or interface pool)
(actually, in practice, I guess it shouldn't be any more trouble to do fine grained access control, as long as you can make the decision of whether or not to allow later on in the function that implements the API (at step (6) above), rather than at the top level when the API is first called).
I'm leaning towards (3). If it raises a technical boundary, or people don't like the idea of having an arg in the public API that takes an XPath expression, then maybe I could switch to (2).
Any opinions on which direction I should take? Some way different from what I'm suggesting?
Specific questions:
1) Does having a single public API that is used to update many different parts of the object raise any hurdles wrt. fine grained access control? (knowing that there will be "some point" in that code that we know exactly what the user is attempting to change).
2) any problems / reservations about accepting XPatch expressions in the args of a public API?
Yep, see above.
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. Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
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. 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>", VIR_NETWORK_UPDATE_ADD | VIR_NETWORK_UPDATE_LIVE | VIR_NETWORK_UPDATE_CONFIG); Not sure I like it any better, but at least it's more targetted than option 1 (you can't change arbitrary code, just the section highlighted by the enum value and the choice of add, remove, or replace), so the diff'ing is a bit easier. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
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?
On Fri, Sep 07, 2012 at 11:51:39AM -0400, Laine Stump wrote:
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?
That would work for me. My only point of concern is index, and maybe behaviour of edge cases. If I tweak your first example: virNetworkUpdate(net, VIR_NETWORK_SECTION_IP_DHCP_HOST, 0, /* just enough to match existing entry */ "<host/>", VIR_NETWORK_UPDATE_DELETE | VIR_NETWORK_UPDATE_LIVE | VIR_NETWORK_UPDATE_CONFIG); This may match multiple entries, do we preserve order of those entries ? If not that mean deleting one of the host at random. Is that okay ? I would think yes. Would that be equivalent to virNetworkUpdate(net, VIR_NETWORK_SECTION_IP_DHCP_HOST, 0, NULL, VIR_NETWORK_UPDATE_DELETE | VIR_NETWORK_UPDATE_LIVE | VIR_NETWORK_UPDATE_CONFIG); I would assume yes in practice, the selector would be enough to tell what kind of inforamtions we are trying to remove. I guess looking at some early patch on how the matching is to be done based on the information given by the users, will tell us if that approach is a nightmare or ends up being decent. If trying to lookup the information based on the xml turns horrible, then maybe fallback to XPath (and make clear that we won't support namespaces) I would assume that there are multiple VIR_NETWORK_UPDATE_... flags VIR_NETWORK_UPDATE_DELETE VIR_NETWORK_UPDATE_ADD_LAST VIR_NETWORK_UPDATE_ADD_FIRST VIR_NETWORK_UPDATE_EXISTING i.e. don't try to infer from the matching whether it is an addition or a modification. Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/
participants (4)
-
Daniel P. Berrange -
Daniel Veillard -
Eric Blake -
Laine Stump