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