Re: [Xen-devel] [PATCH v3 3/8] libxl: implement virDomainInterfaceStats

[Joao Martins <joao.m.martins@xxxxxxxxxx>]

On 11/17/2015 11:38 PM, Jim Fehlig wrote:
> Joao Martins wrote:
>>
>> On 11/17/2015 02:48 AM, Jim Fehlig wrote:
>>> On 11/13/2015 06:14 AM, Joao Martins wrote:
>>>> Introduce support for domainInterfaceStats API call for querying
>>>> network interface statistics. Consequently it also enables the
>>>> use of `virsh domifstat <dom> <interface name>` command.
>>>>
>>>> After succesful guest creation we fill the network
>>>> interfaces names based on domain, device id and append suffix
>>>> if it's emulated in the following form: vif<domid>.<devid>[-emu]. Because
>>>> we need the devid from domain config (which is filled by libxl on domain
>>>> create)  we cannot do generate the names in console callback.
>>> Bummer, but see below.
>>>
>>>>  On domain
>>>> cleanup we also clear ifname, in case it was set by libvirt (i.e.
>>>> being prefixed with "vif"). We also skip these two steps in case the name
>>>> of the interface was manually inserted by the adminstrator.
>>>>
>>>> For getting the interface statistics we resort to virNetInterfaceStats
>>>> and let libvirt handle the platform specific nits. Note that the latter
>>>> is not yet supported in FreeBSD.
>>>>
>>>> Signed-off-by: Joao Martins <joao.m.martins@xxxxxxxxxx>
>>>> ---
>>>> Changes since v2:
>>>>  - Clear ifname if it's autogenerated, since otherwise will persist
>>>>  on successive domain starts. Change commit message reflecting this
>>>>  change.
>>>>
>>>> Changes since v1:
>>>>  - Fill <virDomainNetDef>.ifname after domain start with generated
>>>>  name from libxl  based on domain id and devid returned by libxl.
>>>>  After that path validation don interfaceStats is enterily based
>>>>  on ifname pretty much like the other drivers.
>>>>  - Modify commit message reflecting the changes mentioned in
>>>>  the previous item.
>>>>  - Bump version to 1.2.22
>>>> ---
>>>>  src/libxl/libxl_domain.c | 26 ++++++++++++++++++++++++++
>>>>  src/libxl/libxl_driver.c | 48 
>>>> ++++++++++++++++++++++++++++++++++++++++++++++++
>>>>  2 files changed, 74 insertions(+)
>>>>
>>>> diff --git a/src/libxl/libxl_domain.c b/src/libxl/libxl_domain.c
>>>> index 40dcea1..adb4563 100644
>>>> --- a/src/libxl/libxl_domain.c
>>>> +++ b/src/libxl/libxl_domain.c
>>>> @@ -728,6 +728,17 @@ libxlDomainCleanup(libxlDriverPrivatePtr driver,
>>>>          }
>>>>      }
>>>>  
>>>> +    if ((vm->def->nnets)) {
>>>> +        ssize_t i;
>>>> +
>>>> +        for (i = 0; i < vm->def->nnets; i++) {
>>>> +            virDomainNetDefPtr net = vm->def->nets[i];
>>>> +
>>>> +            if (STRPREFIX(net->ifname, "vif"))
>>>> +                VIR_FREE(net->ifname);
>>> Would not be nice if user-specified ifname started with "vif" :-).
>>>
>> I think QEMU they do in a similar way, in case, specified interfaces started
>> with a prefix that is solely for autogenerated interface naming.
> 
> Ah, right. Same with bhyve and UML drivers.
> 
>> Would you
>> prefer removing it? The problem with removing this is that ifname would 
>> persist
>> on the XML, and future domainStart would use the old value.
> 
> Yep, understood. Fine to leave it.
> 
OK.

>>
>>>> +        }
>>>> +    }
>>>> +
>>>>      if (virAsprintf(&file, "%s/%s.xml", cfg->stateDir, vm->def->name) > 
>>>> 0) {
>>>>          if (unlink(file) < 0 && errno != ENOENT && errno != ENOTDIR)
>>>>              VIR_DEBUG("Failed to remove domain XML for %s", 
>>>> vm->def->name);
>>>> @@ -901,6 +912,7 @@ libxlDomainStart(libxlDriverPrivatePtr driver, 
>>>> virDomainObjPtr vm,
>>>>      virDomainDefPtr def = NULL;
>>>>      virObjectEventPtr event = NULL;
>>>>      libxlSavefileHeader hdr;
>>>> +    ssize_t i;
>>>>      int ret = -1;
>>>>      uint32_t domid = 0;
>>>>      char *dom_xml = NULL;
>>>> @@ -1023,6 +1035,20 @@ libxlDomainStart(libxlDriverPrivatePtr driver, 
>>>> virDomainObjPtr vm,
>>>>       */
>>>>      vm->def->id = domid;
>>>>  
>>>> +    for (i = 0; i < vm->def->nnets; i++) {
>>>> +        virDomainNetDefPtr net = vm->def->nets[i];
>>>> +        libxl_device_nic *x_nic = &d_config.nics[i];
>>>> +        const char *suffix =
>>>> +            x_nic->nictype != LIBXL_NIC_TYPE_VIF ? "-emu" : "";
>>>> +
>>>> +        if (net->ifname)
>>>> +            continue;
>>>> +
>>>> +        if (virAsprintf(&net->ifname, "vif%d.%d%s",
>>>> +                        domid, x_nic->devid, suffix) < 0)
>>>> +            continue;
>>>> +    }
>>>> +
>>> This could be done in the callback, if we added the libxl_domain_config 
>>> object
>>> to the libxlDomainObjPrivate struct. Currently we create, use, and dispose 
>>> the
>>> object in libxlDomainStart, but it would probably be useful keep the object
>>> around while the domain is active.
>> That's a good idea. This chunk would definitely be better placed in 
>> ConsoleCallback.
> 
> Agreed, with ConsoleCallback being renamed to something a bit more generic 
> like
> DomainStartCallback or similar.
> 
OK.

>>
>>> Actually, you could directly use the object
>>> in libxlDomainInterfaceStats if it was included in the libxlDomainObjPrivate
>>> struct. I realize it is a bit more work than this or the V1 approach, but 
>>> what
>>> do you think about it?
>> Huum, IIUC we would be doing similar to V1, with the difference that we 
>> would do
>> it more reliably by knowning devid in advance through usage of
>> libxl_domain_config. The good thing about this version though is that it
>> simplifies things simple for interfaceStats and getAllDomainStats, since all 
>> it
>> takes is just to compare against the interface name we calculated beforehand 
>> on
>> domain start. Moreover we can actually see what interfaces each domain has 
>> when
>> doing domiflist as a result of setting net->ifname (right now only "-" would
>> show up in the name).
> 
> Yes, good point. I think it is best to generate the name when starting the VM.
> Other drivers take the same approach.
> 
> So it looks like this simple patch could become a series in itself: one patch
> adding libxl_domain_config to the libxlDomainObjPrivate struct, another patch 
> to
> rename ConsoleCallback to a more generic name, and finally this patch
> implementing virDomainInterfaceStats on top of the others.
Cool, Thanks for the guideline. Will send it over this new series!

Regards,
Joao

> 
> Regards,
> Jim
> 

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel