Connecting a tap device to an Open vSwitch is done by adding a "port"
to the switch with the ovs-vsctl "add-port" command. The port will
have the same name as the tap device, but it is a separate entity, and
can survive beyond the destruction of the tap device (although under
normal circumstances the port will be deleted around the same time the
tap device is deleted).

This makes it possible for a port of a particular name to already
exist at the time libvirt calls ovs-vsctl to add that port. The
original commit of Open vSwitch support (commit df81004632, libvirt
0.9.10, Feb. 2012) used the "--may-exist" option to the add-port
command to indicate that a port of the desired name might already
exist, and that it was okay to simply re-use this port (rather than
failing with an error message).

Then in commit 33445ce8446d9 (libvirt 1.2.7, April 2014) the command
was changed to use "--if-exists del-port blah" instead of
"--may-exist". The reason given was that there was a bug in OVS where
a stale port would be unusable even though it still existed; the
workaround was to forcibly delete any existing port prior to adding
the new port (of the same name). This is the ovs-vsctl command still
in use by libvirt today.

It recently came up in the discussion of a bug concerning guest packet
loss during OpenStack upgrades (https://bugzilla.redhat.com/1963164)
that the bug in OVS that necessitated the del-port workaround was
fixed quite a long time ago (August 2015):

  https://github.com/openvswitch/ovs/commit/e21c6643a02c6b446d2fbdfde366ea303b4c2730

thus rendering the workaround in libvirt unnecessary. The assertion in
that discussion is that this workaround is now the cause of the packet
loss being experienced during OpenStack upgrades. I'm not convinced
this is the case, but it does appear that there is no reason to carry
this workaround in libvirt any longer, so this patch reverts the code
back to the original behavior (using "--may-exist" instead of
"--if-exists del-port").

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

In an attempt to sabotage my own patch, I will point out that this
makes it a possibility that libvirt could end up using an OVS port
created by "someone else" who may have set it up in some way that
renders the guest network connection unusable, e.g. if the port had
been created with flow filters or something. On the other hand 1) the
"someone else" in this scenario would need to have all the same
privileges that would also permit them to mess up the connection after
it had been created (so there is no *extra* security risk), and 2)
this could possibly be *desired* (although difficult for libvirt to
support) behavior in the case someone needed libvirt-managed guest
network connections to use extra capabilities not directly supported
in libvirt's config; additionally pre-creating the OVS port could
provide a method of getting the network connection into a usable state
(i.e. forwarding traffic) much sooner.


src/util/virnetdevopenvswitch.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/src/util/virnetdevopenvswitch.c b/src/util/virnetdevopenvswitch.c
index 21ee4bdd42..eac68d9556 100644
--- a/src/util/virnetdevopenvswitch.c
+++ b/src/util/virnetdevopenvswitch.c
@@ -155,8 +155,8 @@ int virNetDevOpenvswitchAddPort(const char *brname, const char *ifname,
     }
 
     cmd = virNetDevOpenvswitchCreateCmd();
-    virCommandAddArgList(cmd, "--", "--if-exists", "del-port",
-                         ifname, "--", "add-port", brname, ifname, NULL);
+    virCommandAddArgList(cmd, "--", "--may-exist",
+                         "add-port", brname, ifname, NULL);
 
     virNetDevOpenvswitchConstructVlans(cmd, virtVlan);
 
-- 
2.31.1

On Mon, Jun 07, 2021 at 05:07:17PM -0400, Laine Stump wrote:
> Connecting a tap device to an Open vSwitch is done by adding a "port"
> to the switch with the ovs-vsctl "add-port" command. The port will
> have the same name as the tap device, but it is a separate entity, and
> can survive beyond the destruction of the tap device (although under
> normal circumstances the port will be deleted around the same time the
> tap device is deleted).
> 
> This makes it possible for a port of a particular name to already
> exist at the time libvirt calls ovs-vsctl to add that port. The
> original commit of Open vSwitch support (commit df81004632, libvirt
> 0.9.10, Feb. 2012) used the "--may-exist" option to the add-port
> command to indicate that a port of the desired name might already
> exist, and that it was okay to simply re-use this port (rather than
> failing with an error message).
> 
> Then in commit 33445ce8446d9 (libvirt 1.2.7, April 2014) the command
> was changed to use "--if-exists del-port blah" instead of
> "--may-exist". The reason given was that there was a bug in OVS where
> a stale port would be unusable even though it still existed; the
> workaround was to forcibly delete any existing port prior to adding
> the new port (of the same name). This is the ovs-vsctl command still
> in use by libvirt today.
> 
> It recently came up in the discussion of a bug concerning guest packet
> loss during OpenStack upgrades (https://bugzilla.redhat.com/1963164)
> that the bug in OVS that necessitated the del-port workaround was
> fixed quite a long time ago (August 2015):
> 
>   https://github.com/openvswitch/ovs/commit/e21c6643a02c6b446d2fbdfde366ea303b4c2730
> 
> thus rendering the workaround in libvirt unnecessary. The assertion in
> that discussion is that this workaround is now the cause of the packet
> loss being experienced during OpenStack upgrades. I'm not convinced
> this is the case, but it does appear that there is no reason to carry
> this workaround in libvirt any longer, so this patch reverts the code
> back to the original behavior (using "--may-exist" instead of
> "--if-exists del-port").
> 
> Signed-off-by: Laine Stump <laine@redhat.com>

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


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 :|