[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH v3 3/8] libxl: implement virDomainInterfaceStats
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
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |