On 4/28/19 4:11 PM, Laine Stump wrote:
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@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. 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.htmlIn 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.
Double Ugh. Not so easy as I'd assumed :-(The problem is that we can't just fix it by always putting the type from the config in the XML - if the config type is "network" and the network has a forward mode that uses IP routing to forward packets then the type stays at "network", but if the network has any other forward mode, then the status XML shows the actual type (bridge, direct, hostdev). Essentially, the forward mode of the network is encoded into the "actual type" of the interface, and it's not available from anywhere else at the time we format the status XML, so we can't "internally change the type, but format 'what we would have set the type to in the past' when we're creating the XML document to output.
I *would like to say* that putting the actual type in the status XML at all is an error, and should be fixed. But it's been that way for nearly 5 years now, and changing it might break something else. Also, the actual type (i.e. status of the interface) isn't available from anywhere else. So my brain is kind of stuck on what is the right thing to do.
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.
In order to preserve compatibility with old virsh, I think we need to keep the interface type as it previously was in the status XML. On the other hand, the way it was is just... bad. Not only does it make it impossible to do a "read transient config. modify, update transient config" of an interface, it is encoding multiple things into a single attribute.
(*This* is a great example of why I always worry excessively over any XML addition/change. For all the good it apparently does...)
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
