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

[Joao Martins <joao.m.martins@oracle.com>]

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

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

> 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). The only problem on the latter is the assumption on
domainCleanup in which all interfaces starting "vif" are assumed to be
autogenerated (since there is no flag/field to tell that on virDomainNetDef).

Cheers,
Joao

> 
> Regards,
> Jim
>