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@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;
/* we also store the bridge device and macTableManager settings
* in iface->data.network.actual->data.bridge for later use
@@ -5454,9 +5450,8 @@ networkBandwidthGenericChecks(virDomainNetDefPtr iface,
virNetDevBandwidthPtr ifaceBand;
unsigned long long old_floor, new_floor;
- if (virDomainNetGetActualType(iface) != VIR_DOMAIN_NET_TYPE_NETWORK &&
- (virDomainNetGetActualType(iface) != VIR_DOMAIN_NET_TYPE_BRIDGE ||
- iface->data.network.actual->data.bridge.brname == NULL)) {
+ if (virDomainNetGetActualType(iface) != VIR_DOMAIN_NET_TYPE_BRIDGE ||
+ iface->data.network.actual->data.bridge.brname == NULL) {
/* This is not an interface that's plugged into a network.
* We don't care. Thus from our POV bandwidth change is allowed. */
return false;
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index c443c881d5..31d8647eee 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -4567,7 +4567,7 @@ processNicRxFilterChangedEvent(virQEMUDriverPtr driver,
syncNicRxFilterDeviceOptions(def->ifname, guestFilter, hostFilter);
}
- if (virDomainNetGetActualType(def) == VIR_DOMAIN_NET_TYPE_NETWORK) {
+ if (virDomainNetGetActualType(def) == VIR_DOMAIN_NET_TYPE_BRIDGE) {
const char *brname = virDomainNetGetActualBridgeName(def);
/* For libivrt network connections, set the following TUN/TAP network
--
2.20.1
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-listOn 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@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.
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?
Michal
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
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@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.
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@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
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@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.
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
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@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...)
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On 4/17/19 1: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.
Yeah, I don't remember if there was an operational difference between
the two in the past (other than bandwidth floor) or if I just made the
distinction because I was concerned that there *might be*, but
definitely it's something that needs to go! (Heck, maybe there *still*
is a reason for differentiating, but even if there is we should still be
able to look at the config of the network itself to learn the necessary
details).
>
> This doesn't affect user visible XML since the "actual" device XML
> is internal only, but we need code to convert the data upgrades.
This does mean that downgrades will create "strange situations". Nothing
horribly bad, but worth remembering for future reference.
>
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
> src/conf/domain_conf.c | 28 +++++++++++++++++++++++-----
> src/network/bridge_driver.c | 11 +++--------
> src/qemu/qemu_driver.c | 2 +-
You also need to update the result of several xml2xml tests:
qemustatusxml2xmldata/migration-out-nbd-out.xml
qemustatusxml2xmldata/migration-in-params-out.xml
qemustatusxml2xmldata/migration-out-params-out.xml
qemustatusxml2xmldata/migration-out-nbd-tls-out.xml
qemustatusxml2xmldata/disk-secinfo-upgrade-out.xml
(thanks again to crobinson for VIR_TEST_REGENERATE_OUTPUT :-)
> 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;
> + }
> +
It looks like git was confused here during a rebase, and it merged the
code into the wrong function - this patch puts it into
virDomainDiskDefPostParse(), but I'm guessing it should go in
virDomainNetDefPostParse() (moving it there certainly gets it to compile
properly at least :-)
(NB: once several years ago git merged a chunk into the wrong function
and I learned that (at that time, hopefully it's changed since) git
ignored the name of the function listed just after the line number info
at the top of each chunk).
> 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;
>
> /* we also store the bridge device and macTableManager settings
> * in iface->data.network.actual->data.bridge for later use
> @@ -5454,9 +5450,8 @@ networkBandwidthGenericChecks(virDomainNetDefPtr iface,
> virNetDevBandwidthPtr ifaceBand;
> unsigned long long old_floor, new_floor;
>
> - if (virDomainNetGetActualType(iface) != VIR_DOMAIN_NET_TYPE_NETWORK &&
> - (virDomainNetGetActualType(iface) != VIR_DOMAIN_NET_TYPE_BRIDGE ||
> - iface->data.network.actual->data.bridge.brname == NULL)) {
> + if (virDomainNetGetActualType(iface) != VIR_DOMAIN_NET_TYPE_BRIDGE ||
> + iface->data.network.actual->data.bridge.brname == NULL) {
> /* This is not an interface that's plugged into a network.
> * We don't care. Thus from our POV bandwidth change is allowed. */
> return false;
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index c443c881d5..31d8647eee 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -4567,7 +4567,7 @@ processNicRxFilterChangedEvent(virQEMUDriverPtr driver,
> syncNicRxFilterDeviceOptions(def->ifname, guestFilter, hostFilter);
> }
>
> - if (virDomainNetGetActualType(def) == VIR_DOMAIN_NET_TYPE_NETWORK) {
> + if (virDomainNetGetActualType(def) == VIR_DOMAIN_NET_TYPE_BRIDGE) {
> const char *brname = virDomainNetGetActualBridgeName(def);
>
> /* For libivrt network connections, set the following TUN/TAP network
This at first seemed really short since there are many more places that
check for actualtype=network, but you add an error to all those places
in the next patch, which makes sense.
Assuming that you fix the xml2xml test cases and move the chunk that got
merged into the wrong place:
Reviewed-by: Laine Stump <laine@laine.org>
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
© 2016 - 2026 Red Hat, Inc.