12:14 p.m.
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(a)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.
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