Re: [libvirt] [PATCH v4 04/29] network: use 'bridge' as actual type instead of 'network'

On 4/26/19 5:07 AM, 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(a)redhat.com&gt; >> --- >>   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. 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. Ugh. The problem here is that we've always (or at least *I've* always) worked with the mistaken idea that there are 2 types of info about a domain - status and persistent config) - when really there are 3 - status, transient config, and persistent config. The problem being suffered is because the interface type returned in the live XML has in the past been partly status, partly transient config. Dan's patch has made it 100% status, but the virsh commands above treat it as transient config. I knew I was responsible for the change that made the status XML give the actual type of the interface (so "status") rather than the type from the persistent config, but couldn't remember why I did it, so I looked back in git history and found commit 7d5bf4847. A simpler way to look at it might be via the email from the list (that way you can see the review responses): https://www.redhat.com/archives/libvir-list/2014-February/msg01377.html In short, it was done so that the network hook "plugged" event (and other users of the public API) could get all the information about the way the network device had been setup. It seemed like a good idea at the time, but in retrospect, I think it was a mistake (in this case and in general) to provide status in the same attribute used for config when those two things might be different (when I say it out loud that way, it's pretty obious :-/). Instead, any "status" item that could *ever* possibly differ from a similar "transient config" item should have its own distinct read-only attribute/element in the XML. Once the rest of Dan's "virNetworkPort" series is in, a user will be able to get all the info they need by first getting the domain XML, then using the uuid of the interface to get the NetworkPort XML for that connection (that may still not be perfect, because interfaces that aren't type='network' won't have a networkport, but it will be enough for the use case we're considering here). It will be more complicated that just reading the domain XML a single time, but at least it will allow us to learn status info distinct from transient config. > > 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. Doesn't it all come down to the fact that the status XML now has "bridge" instead of "network"? Maybe if we make a patch to change *only* what is put in the externally visible status XML, but internally (and in the <actual> element of the status that's stored on disk) maintain the change from "network" to "bridge"? I can try making a patch like that.

> > Should we revert this? If we can't do something to get these virsh commands working before the release, then I think we'll have to.