[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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

On 08/21/2012 11:47 PM, Daniel Veillard wrote:
> 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 */
>>    /* delete existing object(s) that matches the xml */
>>    /* take effect immediately */
>>    /* persistent change */
>> 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='' ...>'s dhcp element, you would do this:
>>     /* nodespec obviously uses an XPath expression */
>>     virNetworkUpdate("/ip[address='']/dhcp",
>>                      /* full text of <host> element to add */
>>                      "<host mac='00:16:3e:77:e2:ed' "
>>                      "name='X.example.com' ip=''/>"
>>                      | 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>"
>> To delete a static dhcp host entry, you would use this:
>>     virNetworkUpdate("/ip[address='']/dhcp",
>   shouldn't that be //ip[address='']/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_CONFIG);
>> or if you preferred:
>>     virNetworkUpdate("/ip[address='']/dhcp",
>>                      /* again, just enough to match */
>>                      "<host ip=''/>",
>>                      | VIR_NETWORK_UPDATE_CONFIG);
>> To remove an interface from the interface pool:
>>     virNetworkUpdate("/forward",
>>                      "<interface dev='eth20'/>",
>>                      | 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='' netmask=''>
      <range start='' end='' />
        <dnsmasq:arg value='--max-ttl'/>
        <dnsmasq:arg value='25400'/>
        <dnsmasq:arg value='--resolv-file=/etc/libvirt-resolv.conf'/>

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

> 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.

[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]