[libvirt] [PATCH v4 04/29] network: use 'bridge' as actual type instead of 'network'
Laine Stump
laine at laine.org
Mon Apr 29 14:33:16 UTC 2019
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 at 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.
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...)
More information about the libvir-list
mailing list