[libvirt] [PATCH v3 3/8] libxl: implement virDomainInterfaceStats

[Jim Fehlig]

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 at oracle.com>
>>> ---
>>> 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.

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

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

Regards,
Jim