On Fri, Apr 26, 2019 at 11:07:49AM +0200, Michal Privoznik wrote:
> On 4/17/19 7:19 PM, Daniel P. Berrangé wrote:
> > Ports allocated on virtual networks with type=nat|route|open all get
> > given an actual type of 'network'.
> >
> > Only ports in networks with type=bridge use an actual type of 'bridge'.
> >
> > This distinction makes little sense since the virtualization drivers
> > will treat both actual types in exactly the same way, as they're all
> > just bridge devices a VM needs to be connected to.
> >
> > This doesn't affect user visible XML since the "actual" device XML
> > is internal only, but we need code to convert the data upgrades.
> >
> > Signed-off-by: Daniel P. Berrangé <berrange@xxxxxxxxxx>
> > ---
> > src/conf/domain_conf.c | 28 +++++++++++++++++++++++-----
> > src/network/bridge_driver.c | 11 +++--------
> > src/qemu/qemu_driver.c | 2 +-
> > 3 files changed, 27 insertions(+), 14 deletions(-)
> >
> > diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> > index 0df3c2ed49..1dde45a6fd 100644
> > --- a/src/conf/domain_conf.c
> > +++ b/src/conf/domain_conf.c
> > @@ -5081,6 +5081,19 @@ virDomainDiskDefPostParse(virDomainDiskDefPtr disk,
> > return -1;
> > }
> > + /* Older libvirtd uses actualType==network, but we now
> > + * just use actualType==bridge, as nothing needs to
> > + * distinguish the two cases, and this simplifies virt
> > + * drive code */
> > + if (net->type == VIR_DOMAIN_NET_TYPE_NETWORK &&
> > + net->data.network.actual != NULL &&
> > + net->data.network.actual->type == VIR_DOMAIN_NET_TYPE_NETWORK) {
> > + char mac[VIR_MAC_STRING_BUFLEN];
> > + virMacAddrFormat(&net->mac, mac);
> > + VIR_DEBUG("Updating NIC %s actual type to bridge", mac);
> > + net->data.network.actual->type = VIR_DOMAIN_NET_TYPE_BRIDGE;
> > + }
> > +
> > return 0;
> > }
> > @@ -11267,11 +11280,16 @@ virDomainActualNetDefParseXML(xmlNodePtr node,
> > }
> > bandwidth_node = virXPathNode("./bandwidth", ctxt);
> > - if (bandwidth_node &&
> > - virNetDevBandwidthParse(&actual->bandwidth,
> > - bandwidth_node,
> > - def->type == VIR_DOMAIN_NET_TYPE_NETWORK) < 0)
> > - goto error;
> > + if (bandwidth_node) {
> > + bool allowFloor =
> > + (actual->type == VIR_DOMAIN_NET_TYPE_NETWORK) ||
> > + (actual->type == VIR_DOMAIN_NET_TYPE_BRIDGE &&
> > + actual->data.bridge.brname != NULL);
> > + if (virNetDevBandwidthParse(&actual->bandwidth,
> > + bandwidth_node,
> > + allowFloor) < 0)
> > + goto error;
> > + }
> > vlanNode = virXPathNode("./vlan", ctxt);
> > if (vlanNode && virNetDevVlanParse(vlanNode, ctxt, &actual->vlan) < 0)
> > diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
> > index 6ed0bf1e8e..4055939ade 100644
> > --- a/src/network/bridge_driver.c
> > +++ b/src/network/bridge_driver.c
> > @@ -4491,11 +4491,7 @@ networkAllocateActualDevice(virNetworkPtr net,
> > case VIR_NETWORK_FORWARD_NAT:
> > case VIR_NETWORK_FORWARD_ROUTE:
> > case VIR_NETWORK_FORWARD_OPEN:
> > - /* for these forward types, the actual net type really *is*
> > - * NETWORK; we just keep the info from the portgroup in
> > - * iface->data.network.actual
> > - */
> > - iface->data.network.actual->type = VIR_DOMAIN_NET_TYPE_NETWORK;
> > + iface->data.network.actual->type = VIR_DOMAIN_NET_TYPE_BRIDGE;
>
> Actually, this breaks a lot of stuff. Firstly, for a live XML the type is
> changed to <interface type='bridge'> .. </interface>. This is because
> virDomainNetDefFormat() decides to format actual info.
Urgh this is a real pain point & really a regression. I believed the
comments that the Actual def was internal only and not visible to users.
I think this alone makes a revert necceessary and probably meansI have
to abandon this idea completely :-(
> Having said that,
> transitionally some APIs are broken too. For instance, I have the following
> interface for my domain (inactive XML):
>
> <interface type='network' trustGuestRxFilters='yes'>
> <mac address='52:54:00:a4:6f:91'/>
> <source network='default'/>
> <bandwidth>
> <inbound average='1024' peak='4096' floor='500' burst='1024'/>
> <outbound average='10240'/>
> </bandwidth>
> <model type='virtio'/>
> <mtu size='9000'/>
> </interface>
>
> Now, 'virsh domif-setlink', 'virsh update-device', 'virsh domiftune' don't
> work, because at runtime type is changed to 'bridge' and we don't implement
> many features for that network type.
>
> I'll post patches for the first two issues I've found. But they look pretty
> hacky and I'm not sure my testing was exhaustive enough to find other cases
> where this will break. The third one I have no idea how to fix because it
> boils down to looking a network in the bridge driver but we don't know the
> network name because we don't parse it for interface type bridge.
>
> Should we revert this?
I think reverting is going to be the easiest, certainly for this
imminent release.
Regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
