Re: [PATCH v2 2/2] libxl: implement virDomainInterfaceStats

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Wed, Dec 02, 2015 at 11:50:41AM -0700, Jim Fehlig wrote:
> On 12/02/2015 06:02 AM, Joao Martins wrote:
> >
> > On 12/02/2015 12:45 AM, Jim Fehlig wrote:
> >> On 11/23/2015 11:57 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 plus
> >>> seeing the interfaces names instead of "-" when doing
> >>> `virsh domiflist <dom>`.
> >>>
> >>> After successful 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].
> >> One interesting Xen behavior that has existing for many, many years is that a PV
> >> nic is implicitly created for each emulated nic specified in the config. The
> >> guest OS picks which one to use. These days most will use the PV nic, and if
> >> they are nice, "unplug" the emulated one via the unplug protocol. E.g. an HVM
> >> guest with
> >>
> >>  <interface type='bridge'>
> >>     <source bridge='br0'/>
> >>     <mac address='00:16:3e:7a:35:ce'/>
> >>     <script path='/etc/xen/scripts/vif-bridge'/>
> >>   </interface>
> >>
> >> results in two vif devices on the host
> >>
> >> # ip a | grep vif
> >> 607: vif519.0-emu: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 qdisc pfifo_fast
> >> master br0 state UNKNOWN group default qlen 500
> >> 608: vif519.0: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 qdisc pfifo_fast
> >> master br0 state UNKNOWN group default qlen 512
> >>
> >> both are connected to the bridge
> >>
> >> # brctl show br0
> >> bridge name    bridge id        STP enabled   interfaces
> >> br0        8000.001e676598f5    no            eth0
> >>                                               vif519.0
> >>                                               vif519.0-emu
> >>
> >> In this case, the (not nice) guest OS is using the PV nic but did not unplug the
> >> emulated one. So we have two interfaces, but the virDomainDef only contains one
> >>
> >> # virsh domiflist 519
> >> Interface  Type       Source     Model       MAC
> >> -------------------------------------------------------
> >> vif519.0-emu bridge     br0        -           00:16:3e:7a:35:ce
> >>
> >> Not a fault of this patch, but we'll need to figure out how to handle the
> >> implicitly created PV nic. The interesting case is identifying emulated nics
> >> that have been unplugged by a nice guest, and hence no longer exist in the host
> >> (e.g. vif519.0-emu in the above example).
> >>
> > Indeed this is an issue I am aware of but wasn't sure on how to handle it. The
> > way I test an HVM guest on libvirt with a (explicitly created) PV nic was with
> > "model=netfront" since there wouldn't be an emulated nic.
> 
> Yep, libxl allows us specify a PV nic only, but not an emulated nic only.
> 
> >  So perhaps we could
> > differ it if this was specified on HVM guests. If that wasn't the case we would
> > add the complementary interface along with checking if indeed the net device
> > exists. On cleanup we would just delete the second interface that would appear
> > with the same name.
> 
> Right. So the running config would have the implicitly added PV device. The
> persistent config wouldn't change. Are you volunteering to write a patch? :-).
>
Yeap :-D

> >
> >>> We extract the network interfaces info from libxl in
> >>> libxlDomainStartCallback() and make ifname . 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 v3:
> >>>  - Use libxl_device_nic_list() for getting each network interface
> >>>  devid in DomainStartCallback.
> >>>  - Improve error reporting by appropriately setting the right error
> >>>  when no interface is known.
> >>>  - Do not unlock vm if libxlDomainObjEndJob() returns false
> >>>  - Set vm->def->net[i]->ifname on DomainStartCallback instead of
> >>>  DomainStart.
> >>>  - Change commit message reflecting the changes on the previous
> >>>  item and mention correct interface names when doing domiflist.
> >>>
> >>> 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 | 29 +++++++++++++++++++++++++++
> >>>  src/libxl/libxl_driver.c | 52 ++++++++++++++++++++++++++++++++++++++++++++++++
> >>>  2 files changed, 81 insertions(+)
> >>>
> >>> diff --git a/src/libxl/libxl_domain.c b/src/libxl/libxl_domain.c
> >>> index a7267b0..141f241 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;
> >> size_t
> >>
> > Apologies for the typo.
> >
> >>> +
> >>> +        for (i = 0; i < vm->def->nnets; i++) {
> >>> +            virDomainNetDefPtr net = vm->def->nets[i];
> >>> +
> >>> +            if (STRPREFIX(net->ifname, "vif"))
> >>> +                VIR_FREE(net->ifname);
> >>> +        }
> >>> +    }
> >>> +
> >>>      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);
> >>> @@ -857,6 +868,8 @@ static void
> >>>  libxlDomainStartCallback(libxl_ctx *ctx, libxl_event *ev, void *for_callback)
> >>>  {
> >>>      virDomainObjPtr vm = for_callback;
> >>> +    libxl_device_nic *nics;
> >>> +    int nnics;
> >>>      size_t i;
> >>>  
> >>>      virObjectLock(vm);
> >>> @@ -883,6 +896,22 @@ libxlDomainStartCallback(libxl_ctx *ctx, libxl_event *ev, void *for_callback)
> >>>              VIR_FREE(console);
> >>>          }
> >>>      }
> >>> +
> >>> +    if ((nics = libxl_device_nic_list(ctx, ev->domid, &nnics)) != NULL) {
> >>> +        for (i = 0; i < vm->def->nnets && i < nnics; i++) {
> >>> +            virDomainNetDefPtr net = vm->def->nets[i];
> >>> +            libxl_device_nic *x_nic = &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",
> >>> +                            ev->domid, x_nic->devid, suffix) < 0)
> >>> +                continue;
> >>> +        }
> >>> +    }
> >> As mentioned in my reply to 1/2, I think we should simply create the names after
> >> a successful libxl_domain_create_{new,restore}. Aside from the implicit PV nic
> >> issue, the patch works for me with the below diff squashed in. What do you think
> >> of this change? Does it work for you?
> >>
> > Looks great, Perhaps just changing the version to 1.3.0 since it's no longer 1.2.22.
> 
> I changed that when resolving a conflict that arose from dropping patch 1. I'm
> going to push this patch with my diff squashed in. It has been on the list for
> quite some time, has actually went through 4 versions as your patch history
> notes indicate, and IMO has been well vetted and tested.I think it is an
> appropriate patch to push even though we entered the 1.3.0 freeze today.
> 
Great! Many thanks for the time reviewing it!

> Thanks a lot for your patience!
> 
> Regards,
> Jim
> 
> >
> > Regards,
> > Joao
> >
> > diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c
> > index b3987b9..aa81570 100644
> > --- a/src/libxl/libxl_driver.c
> > +++ b/src/libxl/libxl_driver.c
> > @@ -5472,7 +5472,7 @@ static virHypervisorDriver libxlHypervisorDriver = {
> >  #endif
> >      .nodeGetFreeMemory = libxlNodeGetFreeMemory, /* 0.9.0 */
> >      .nodeGetCellsFreeMemory = libxlNodeGetCellsFreeMemory, /* 1.1.1 */
> > -    .domainInterfaceStats = libxlDomainInterfaceStats, /* 1.2.22 */
> > +    .domainInterfaceStats = libxlDomainInterfaceStats, /* 1.3.0 */
> >      .domainMemoryStats = libxlDomainMemoryStats, /* 1.3.0 */
> >      .domainGetCPUStats = libxlDomainGetCPUStats, /* 1.3.0 */
> >      .connectDomainEventRegister = libxlConnectDomainEventRegister, /* 0.9.0 */
> >
> >
> >> Regards,
> >> Jim
> >>
> >>
> >> diff --git a/src/libxl/libxl_domain.c b/src/libxl/libxl_domain.c
> >> index b45cd27..086406b 100644
> >> --- a/src/libxl/libxl_domain.c
> >> +++ b/src/libxl/libxl_domain.c
> >> @@ -729,7 +729,7 @@ libxlDomainCleanup(libxlDriverPrivatePtr driver,
> >>      }
> >>  
> >>      if ((vm->def->nnets)) {
> >> -        ssize_t i;
> >> +        size_t i;
> >>  
> >>          for (i = 0; i < vm->def->nnets; i++) {
> >>              virDomainNetDefPtr net = vm->def->nets[i];
> >> @@ -868,8 +868,6 @@ static void
> >>  libxlConsoleCallback(libxl_ctx *ctx, libxl_event *ev, void *for_callback)
> >>  {
> >>      virDomainObjPtr vm = for_callback;
> >> -    libxl_device_nic *nics;
> >> -    int nnics;
> >>      size_t i;
> >>  
> >>      virObjectLock(vm);
> >> @@ -897,25 +895,35 @@ libxlConsoleCallback(libxl_ctx *ctx, libxl_event *ev, void
> >> *for_callback)
> >>          }
> >>      }
> >>  
> >> -    if ((nics = libxl_device_nic_list(ctx, ev->domid, &nnics)) != NULL) {
> >> -        for (i = 0; i < vm->def->nnets && i < nnics; i++) {
> >> -            virDomainNetDefPtr net = vm->def->nets[i];
> >> -            libxl_device_nic *x_nic = &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",
> >> -                            ev->domid, x_nic->devid, suffix) < 0)
> >> -                continue;
> >> -        }
> >> -    }
> >>      virObjectUnlock(vm);
> >>      libxl_event_free(ctx, ev);
> >>  }
> >>  
> >> +/*
> >> + * Create interface names for the network devices in parameter def.
> >> + * Names are created with the pattern 'vif<domid>.<devid><suffix>'.
> >> + * devid is extracted from the network devices in the d_config
> >> + * parameter. User-provided interface names are skipped.
> >> + */
> >> +static void
> >> +libxlDomainCreateIfaceNames(virDomainDefPtr def, libxl_domain_config *d_config)
> >> +{
> >> +    size_t i;
> >> +
> >> +    for (i = 0; i < def->nnets && i < d_config->num_nics; i++) {
> >> +        virDomainNetDefPtr net = 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;
> >> +
> >> +        ignore_value(virAsprintf(&net->ifname, "vif%d.%d%s",
> >> +                                 def->id, x_nic->devid, suffix));
> >> +    }
> >> +}
> >> +
> >>  
> >>  /*
> >>   * Start a domain through libxenlight.
> >> @@ -1056,6 +1064,7 @@ libxlDomainStart(libxlDriverPrivatePtr driver,
> >> virDomainObjPtr vm,
> >>      if (libxl_evenable_domain_death(cfg->ctx, vm->def->id, 0, &priv->deathW))
> >>          goto cleanup_dom;
> >>  
> >> +    libxlDomainCreateIfaceNames(vm->def, &d_config);
> >>  
> >>      if ((dom_xml = virDomainDefFormat(vm->def, 0)) == NULL)
> >>          goto cleanup_dom;
> >> j
> >>
> 

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list



[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]