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