3:11 p.m.
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>
> ---
> 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.