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

Re: [libvirt] [PATCH 7/7] network: include plugged interface XML in "plugged" network hook



On 02/24/2014 06:45 PM, Michal Privoznik wrote:
> On 21.02.2014 14:58, Laine Stump wrote:
>> The network hook script gets called whenever an interface is plugged
>> into or unplugged from a network, but even though the full XML of both
>> the network and the domain is included, there is no reasonable way to
>> determine what exact resources the plugged interface is using:
>>
>> 1) Prior to a recent patch which modified the status XML of interfaces
>> to include the information about actual hardware resources used, it
>> would be possible to scan through the domain XML output sent to the
>> hook, and from there find the correct interface, but that interface
>> definition would not include any runtime info (e.g. bandwidth or vlan
>> taken from a portgroup, or which physdev was used in case of a macvtap
>> network).
>>
>> 2) After the patch modifying the status XML of interfaces, the network
>> name would no longer be included in the domain XML, so it would be
>> completely impossible to determine which interface was the one being
>> plugged.
>>
>> To solve that problem, this patch includes a single <interface>
>> element at the beginning of the XML sent to the network hook for
>> "plugged" and "unplugged" (just inside <hookData>) that is the status
>> XML of the interface being plugged. This XML will include all info
>> gathered from the chosen network and portgroup.
>>
>> NB: due to hardcoded spaces in all of the device *Format() functions,
>> the <interface> element inside the <hookData> will be indented by 6
>> spaces rather than 2. I had intended to fix this, but it turns out
>> that to make virDomainNetDefFormat() indentation relative, I would
>> have to do the same to virDomainDeviceInfoFormat(), and that function
>> is called from 19 places - making that a prerequisite of this patch
>> would cause too many merge difficulties if we needed to backport
>> network hooks, so I chose to ignore the problem here and fix the
>> problem for *all* devices in a followup later.
>> ---
>>   src/network/bridge_driver.c | 48 ++++++++++++++++++++++++++++++---------------
>>   1 file changed, 32 insertions(+), 16 deletions(-)
>>
>> diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
>> index a6c719d..152bd06 100644
>> --- a/src/network/bridge_driver.c
>> +++ b/src/network/bridge_driver.c
>> @@ -141,6 +141,7 @@ networkObjFromNetwork(virNetworkPtr net)
>>   static int
>>   networkRunHook(virNetworkObjPtr network,
>>                  virDomainDefPtr dom,
>> +               virDomainNetDefPtr iface,
>>                  int op,
>>                  int sub_op)
>>   {
>> @@ -158,6 +159,8 @@ networkRunHook(virNetworkObjPtr network,
>>   
>>           virBufferAddLit(&buf, "<hookData>\n");
>>           virBufferAdjustIndent(&buf, 2);
>> +        if (iface && virDomainNetDefFormat(&buf, iface, 0) < 0)
>> +            goto cleanup;
>>           if (virNetworkDefFormatBuf(&buf, network->def, 0) < 0)
>>               goto cleanup;
>>           if (dom && virDomainDefFormatInternal(dom, 0, &buf) < 0)
>> @@ -2067,7 +2070,7 @@ networkStartNetwork(virNetworkDriverStatePtr driver,
>>   
>>       /* Run an early hook to set-up missing devices.
>>        * If the script raised an error abort the launch. */
>> -    if (networkRunHook(network, NULL,
>> +    if (networkRunHook(network, NULL, NULL,
>>                          VIR_HOOK_NETWORK_OP_START,
>>                          VIR_HOOK_SUBOP_BEGIN) < 0)
>>           goto cleanup;
>> @@ -2092,7 +2095,7 @@ networkStartNetwork(virNetworkDriverStatePtr driver,
>>       }
>>   
>>       /* finally we can call the 'started' hook script if any */
>> -    if (networkRunHook(network, NULL,
>> +    if (networkRunHook(network, NULL, NULL,
>>                          VIR_HOOK_NETWORK_OP_STARTED,
>>                          VIR_HOOK_SUBOP_BEGIN) < 0)
>>           goto cleanup;
>> @@ -2158,7 +2161,7 @@ static int networkShutdownNetwork(virNetworkDriverStatePtr driver,
>>       }
>>   
>>       /* now that we know it's stopped call the hook if present */
>> -    networkRunHook(network, NULL, VIR_HOOK_NETWORK_OP_STOPPED,
>> +    networkRunHook(network, NULL, NULL, VIR_HOOK_NETWORK_OP_STOPPED,
>>                      VIR_HOOK_SUBOP_END);
>>   
>>       network->active = 0;
>> @@ -3659,14 +3662,8 @@ validate:
>>           }
>>       }
>>   
>> -    /* finally we can call the 'plugged' hook script if any */
>> -    if (networkRunHook(network, dom,
>> -                       VIR_HOOK_NETWORK_OP_IFACE_PLUGGED,
>> -                       VIR_HOOK_SUBOP_BEGIN) < 0)
>> -        goto error;
>> -
>>       if (dev) {
>> -        /* we are now assured of success, so mark the allocation */
>> +        /* mark the allocation */
>>           dev->connections++;
>>           if (actualType != VIR_DOMAIN_NET_TYPE_HOSTDEV) {
>>               VIR_DEBUG("Using physical device %s, %d connections",
>> @@ -3684,6 +3681,19 @@ validate:
>>           VIR_DEBUG("Using network %s, %d connections",
>>                     netdef->name, netdef->connections);
>>       }
>> +
>> +    /* finally we can call the 'plugged' hook script if any */
>> +    if (networkRunHook(network, dom, iface,
>> +                       VIR_HOOK_NETWORK_OP_IFACE_PLUGGED,
>> +                       VIR_HOOK_SUBOP_BEGIN) < 0) {
>> +        /* adjust for failure */
>> +        if (dev)
>> +            dev->connections--;
>> +        if (netdef)
>> +            netdef->connections--;
>> +        goto error;
>> +    }
>> +
>>       ret = 0;
>>   
>>   cleanup:
>> @@ -3865,14 +3875,20 @@ networkNotifyActualDevice(virDomainDefPtr dom,
>>       }
>>   
>>   success:
>> -    /* finally we can call the 'plugged' hook script if any */
>> -    if (networkRunHook(network, dom, VIR_HOOK_NETWORK_OP_IFACE_PLUGGED,
>> -                       VIR_HOOK_SUBOP_BEGIN) < 0)
>> -        goto error;
>> -
>>       netdef->connections++;
>>       VIR_DEBUG("Using network %s, %d connections",
>>                 netdef->name, netdef->connections);
>> +
>> +    /* finally we can call the 'plugged' hook script if any */
>> +    if (networkRunHook(network, dom, iface, VIR_HOOK_NETWORK_OP_IFACE_PLUGGED,
>> +                       VIR_HOOK_SUBOP_BEGIN) < 0) {
>> +        /* adjust for failure */
>> +        if (dev)
>> +            dev->connections--;
>> +        netdef->connections--;
>> +        goto error;
>> +    }
>> +
>>       ret = 0;
>>   cleanup:
>>       if (network)
>> @@ -4018,7 +4034,7 @@ success:
>>           netdef->connections--;
>>   
>>       /* finally we can call the 'unplugged' hook script if any */
>> -    networkRunHook(network, dom, VIR_HOOK_NETWORK_OP_IFACE_UNPLUGGED,
>> +    networkRunHook(network, dom, iface, VIR_HOOK_NETWORK_OP_IFACE_UNPLUGGED,
>>                      VIR_HOOK_SUBOP_BEGIN);
>>   
>>       VIR_DEBUG("Releasing network %s, %d connections",
>>
> ACK although the indentation of XML we're passing to the hook script seems off:

Right. That's what the last paragraph of the commit log message is about
- fixing that indentation would require a much more invasive change that
would touch all the other device parsing functions, which could turn any
potential backport into a real headache, so I chose to not fix it in
this series.

>
> <hookData>
>       <interface type='network'>
>       ...
>       </interface>
>   <network>
>       ...
>   </network>
>   <domain type='kvm'>
>   </domain>
> </hookData>
>
> This is not a show stopper for me. I wonder if we should push these patches now, even during the freeze as this is very closely related to the network hooks - one of the main features in this release.

In a way it is a bugfix for that feature (since the functionality of the
"plugged" hook is severely limited without it). I would feel more
comfortable about pushing it, though, if danpb took a look at the commit
log message for patch 5/7 and gave his okay on the change in the XML. My
opinion is that I should have done it this way to begin with, but Dan
has much better insight than I do on what is and isn't good for
management applications.


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