conf: reattach interface taps to correct bridge on restart

[libvirt] [PATCH] conf: reattach interface taps to correct bridge on restart

Posted by Laine Stump 4 years, 7 months ago

When the bridge re-attach handling was moved out of the network driver
and into the hypervisor driver (commit b806a60e) as a part of the
refactor to split the network driver into a separate daemon, the check
was accidentally changed to only check for type='bridge'. The check for
type in this case needs to check for type='network' as well.

(at the time we thought that type='network' and type='bridge' could be
conflated for interface actual type, but this turned out to be too
problematic to do).

Signed-off-by: Laine Stump <laine@redhat.com>
---

(NB: I thought I remembered seeing a bugzilla report go by about this
regression, but couldn't find it in any of the places I frequent
(upstream, Fedora, or RHEL) If someone can locate it and wants to let
me know the BZ#, I can add it to the commit message and update the
report).

(This fixes the reconnect of taps to their bridges, but the count
maintained in the network object still isn't being updated in these
cases. I've tried removing the !virUUIDIsValid() check in this same
chunk, along with preserving the original port uuid if it's already
valid in virDomainNetDefActualToNetworkPort(), and that results in
fixing the usage count for type='network' when it's a libvirt-managed
bridge or a macvtap passthrough pool, but leads to errors in other
cases.)


 src/conf/domain_conf.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 848c831330..24223bceb2 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -30971,13 +30971,16 @@ virDomainNetNotifyActualDevice(virConnectPtr conn,
                                virDomainDefPtr dom,
                                virDomainNetDefPtr iface)
 {
+    virDomainNetType actualType = virDomainNetGetActualType(iface);
+
     if (!virUUIDIsValid(iface->data.network.portid)) {
         if (virDomainNetCreatePort(conn, dom, iface,
                                    VIR_NETWORK_PORT_CREATE_RECLAIM) < 0)
             return;
     }
 
-    if (virDomainNetGetActualType(iface) == VIR_DOMAIN_NET_TYPE_BRIDGE) {
+    if (actualType == VIR_DOMAIN_NET_TYPE_NETWORK ||
+        actualType == VIR_DOMAIN_NET_TYPE_BRIDGE) {
         /*
          * NB: we can't notify the guest of any MTU change anyway,
          * so there is no point in trying to learn the actualMTU
-- 
2.21.0

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH] conf: reattach interface taps to correct bridge on restart

Posted by Daniel P. Berrangé 4 years, 7 months ago

On Fri, Sep 20, 2019 at 10:06:35AM -0400, Laine Stump wrote:
> When the bridge re-attach handling was moved out of the network driver
> and into the hypervisor driver (commit b806a60e) as a part of the
> refactor to split the network driver into a separate daemon, the check
> was accidentally changed to only check for type='bridge'. The check for
> type in this case needs to check for type='network' as well.
> 
> (at the time we thought that type='network' and type='bridge' could be
> conflated for interface actual type, but this turned out to be too
> problematic to do).
> 
> Signed-off-by: Laine Stump <laine@redhat.com>

Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>

> (This fixes the reconnect of taps to their bridges, but the count
> maintained in the network object still isn't being updated in these
> cases. I've tried removing the !virUUIDIsValid() check in this same
> chunk, along with preserving the original port uuid if it's already
> valid in virDomainNetDefActualToNetworkPort(), and that results in
> fixing the usage count for type='network' when it's a libvirt-managed
> bridge or a macvtap passthrough pool, but leads to errors in other
> cases.)

IIUC, we need to do is use something like

   bool reclaim = false;
   if (!virUUIDIsValid(portid)) {
     reclaim = true;
   } else {
     port = virNetworkLookupPortByUUID(net, portid);
     if (port == NULL)
       reclaim = true;
     else
       virObjectUnref(port)
   }

   if (reclaim)
      virDomainNetCreatePort...

> 
> 
>  src/conf/domain_conf.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 848c831330..24223bceb2 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -30971,13 +30971,16 @@ virDomainNetNotifyActualDevice(virConnectPtr conn,
>                                 virDomainDefPtr dom,
>                                 virDomainNetDefPtr iface)
>  {
> +    virDomainNetType actualType = virDomainNetGetActualType(iface);
> +
>      if (!virUUIDIsValid(iface->data.network.portid)) {
>          if (virDomainNetCreatePort(conn, dom, iface,
>                                     VIR_NETWORK_PORT_CREATE_RECLAIM) < 0)
>              return;
>      }
>  
> -    if (virDomainNetGetActualType(iface) == VIR_DOMAIN_NET_TYPE_BRIDGE) {
> +    if (actualType == VIR_DOMAIN_NET_TYPE_NETWORK ||
> +        actualType == VIR_DOMAIN_NET_TYPE_BRIDGE) {
>          /*
>           * NB: we can't notify the guest of any MTU change anyway,
>           * so there is no point in trying to learn the actualMTU
> -- 
> 2.21.0
> 

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