Issue:
Upon updating the network_name option of localnet port from one physical
bridge to another, ovn-controller fails to cleanup the peer localnet
patch port from the old bridge and ends up creating a duplicate peer
localnet patch port which fails in the following ovsdb transaction:
```
"Transaction causes multiple rows in \"Interface\" table to have
identical values
(patch-localnet_a7d47490-8a90-4c4d-8266-2915ad07c185-to-br-int)
```
Current workflow:
1. Keep a set of all existing localnet ports on br-int. Let us call this
set: existing_ports.
2. For each localnet port in SB:
2.1 Create a patch port on br-int. (if it already exists on br-int,
do not create but remove the entry from exisitng_ports set).
2.2 Create a peer patch port on br-phys-x. (if it already exists on the
bridge, do not create but remove the entry from exisitng_ports set).
3. Finally, cleanup all the ports and its peers from exisiting_ports.
With the above workflow, upon network_name change of localnet LSP, since
ports on br-int does not change, only peer port needs to be move from
br-phys-x to br-phys-y, the localnet port is removed from
exisiting_ports in #2.1 and its peer never becomes eligible for cleanup.
Fix:
Include both patch port on br-int and its peer port in the
exisiting_ports set as part of #1.
This make sures that the peer port is only removed from existing_ports
only if it is already present on the correct bridge. (#2.1/#2.2)
Otherwise, during the cleanup in #3 it will be considered.
Fixes: b600316f252a ("Don't delete patch ports that don't belong to br-int")
Signed-off-by: Priyankar Jain <priyankar.j...@nutanix.com>
---
controller/patch.c | 32 +++++++------
tests/ovn.at | 109 +++++++++++++++++++++++++++++++++++++++++++++
2 files changed, 124 insertions(+), 17 deletions(-)
diff --git a/controller/patch.c b/controller/patch.c
index c1cd5060d..4fed6e375 100644
--- a/controller/patch.c
+++ b/controller/patch.c
@@ -314,6 +314,21 @@ patch_run(struct ovsdb_idl_txn *ovs_idl_txn,
|| smap_get(&port->external_ids, "ovn-l3gateway-port")
|| smap_get(&port->external_ids, "ovn-logical-patch-port")) {
shash_add(&existing_ports, port->name, port);
+ /* Also add peer ports to the list. */
+ for (size_t j = 0; j < port->n_interfaces; j++) {
+ struct ovsrec_interface *p_iface = port->interfaces[j];
+ if (strcmp(p_iface->type, "patch")) {
+ continue;
+ }
+ const char *peer_name = smap_get(&p_iface->options, "peer");
+ if (peer_name) {
+ const struct ovsrec_port *peer_port =
+ get_port(ovsrec_port_by_name, peer_name);
+ if (peer_port) {
+ shash_add(&existing_ports, peer_port->name, peer_port);
+ }
+ }
+ }
}
}
@@ -336,23 +351,6 @@ patch_run(struct ovsdb_idl_txn *ovs_idl_txn,
* ovn-controller. Otherwise it may cause unncessary dataplane
* interruption during restart/upgrade. */
if (!daemon_started_recently()) {
- /* delete peer patch port first */
- for (size_t i = 0; i < port->n_interfaces; i++) {
- struct ovsrec_interface *iface = port->interfaces[i];
- if (strcmp(iface->type, "patch")) {
- continue;
- }
- const char *iface_peer = smap_get(&iface->options, "peer");
- if (iface_peer) {
- const struct ovsrec_port *peer_port =
- get_port(ovsrec_port_by_name, iface_peer);
- if (peer_port) {
- remove_port(bridge_table, peer_port);
- }
- }
- }
-
- /* now delete integration bridge patch port */
remove_port(bridge_table, port);
}
}
diff --git a/tests/ovn.at b/tests/ovn.at
index 4d0c7ad53..4b71e4916 100644
--- a/tests/ovn.at
+++ b/tests/ovn.at
@@ -36454,6 +36454,115 @@ OVN_CLEANUP([hv1])
AT_CLEANUP
])
+OVN_FOR_EACH_NORTHD([
+AT_SETUP([ovn-controller update network_name option for localnet port])
+ovn_start
+net_add n1
+
+sim_add hv1
+as hv1
+
+# Bridge Topology
+# Initial: br-int and br-phys-1 connected through ovn-localnet patch port.
+#
+# br-phys-1 -- br-int
+#
+# Final: br-int and br-phys-3 connected through ovn-localnet patch port.
+# Similarly br-int-2 and br-hys-2 connected through localnet patch port
+# but not owned by this controller.
+#
+# br-phys-1 br-int -- br-phys-3
+# br-int-2 -- br-phys-2
+
+# Create bridges
+check ovs-vsctl add-br br-int
+check ovs-vsctl add-br br-int-2
+check ovs-vsctl add-br br-phys-1
+check ovs-vsctl add-br br-phys-2
+check ovs-vsctl add-br br-phys-3
+check ovs-vsctl set open .
external-ids:ovn-bridge-mappings-hv1=phys-1:br-phys-1,phy-2:br-phys-2,phys-3:br-phys-3
+
+ovn_attach n1 br-phys-1 192.168.1.1 24
+
+check ovs-vsctl -- add-port br-int vif1 -- \
+ set interface vif1 external-ids:iface-id=lp1
+
+check ovn-nbctl ls-add ls
+
+check ovn-nbctl lsp-add ls lp1
+check ovn-nbctl lsp-set-addresses lp1 "00:00:00:00:00:01 10.0.0.1"
+
+check ovn-nbctl lsp-add ls ln_port
+check ovn-nbctl lsp-set-addresses ln_port unknown
+check ovn-nbctl lsp-set-type ln_port localnet
+check ovn-nbctl lsp-set-options ln_port network_name=phys-1
+wait_for_ports_up
+check ovn-nbctl --wait=hv sync
+
+# Allow controller to immediately clean patch ports up if any.
+check ovn-appctl -t ovn-controller debug/ignore-startup-delay
+
+# check that patch port created on br-int and br-phys-1.
+OVS_WAIT_UNTIL([
+ test 1 = $(ovs-vsctl --columns _uuid --bare find Port
name=patch-br-int-to-ln_port | wc -l)
+])
+OVS_WAIT_UNTIL([
+ test 1 = $(ovs-vsctl list-ports br-int | grep patch-br-int-to-ln_port | wc
-l)
+])
+OVS_WAIT_UNTIL([
+ test 1 = $(ovs-vsctl --columns _uuid --bare find Port
name=patch-ln_port-to-br-int | wc -l)
+])
+OVS_WAIT_UNTIL([
+ test 1 = $(ovs-vsctl list-ports br-phys-1 | grep patch-ln_port-to-br-int |
wc -l)
+])
+
+check ovn-appctl debug/pause
+
+# Now move the localnet port from phys-1 to phys-3.
+check ovn-nbctl lsp-set-options ln_port network_name=phys-3
+
+# Also create fake localnet ports on br-int-2
+check ovs-vsctl -- add-port br-int-2 fake-int-2-to-phys-2 -- \
+ set port fake-int-2-to-phys-2 external-ids:ovn-localnet-port=fake-port -- \
+ set interface fake-int-2-to-phys-2 options:peer=fake-phys-2-to-int-2
type=patch
+check ovs-vsctl -- add-port br-phys-2 fake-phys-2-to-int-2 -- \
+ set port fake-phys-2-to-int-2 external-ids:ovn-localnet-port=fake-port -- \
+ set interface fake-phys-2-to-int-2 options:peer=fake-int-2-to-phys-2
type=patch
+
+check ovn-appctl debug/resume
+check ovn-nbctl --wait=hv sync
+
+# patch for port on br-phys-1 is cleanedup.
+OVS_WAIT_UNTIL([
+ test 0 = $(ovs-vsctl list-ports br-phys-1 | grep patch-ln_port-to-br-int |
wc -l)
+])
+
+# Patch port created on br-int and br-phy-3
+OVS_WAIT_UNTIL([
+ test 1 = $(ovs-vsctl --columns _uuid --bare find Port
name=patch-br-int-to-ln_port | wc -l)
+])
+OVS_WAIT_UNTIL([
+ test 1 = $(ovs-vsctl list-ports br-int | grep patch-br-int-to-ln_port | wc
-l)
+])
+OVS_WAIT_UNTIL([
+ test 1 = $(ovs-vsctl --columns _uuid --bare find Port
name=patch-ln_port-to-br-int | wc -l)
+])
+OVS_WAIT_UNTIL([
+ test 1 = $(ovs-vsctl list-ports br-phys-3 | grep patch-ln_port-to-br-int |
wc -l)
+])
+
+# check that patch port on a different bridge is not touched
+OVS_WAIT_UNTIL([
+ test 1 = $(ovs-vsctl --columns _uuid --bare find Port
name=fake-int-2-to-phys-2 | wc -l)
+])
+OVS_WAIT_UNTIL([
+ test 1 = $(ovs-vsctl --columns _uuid --bare find Port
name=fake-phys-2-to-int-2 | wc -l)
+])
+
+OVN_CLEANUP([hv1])
+AT_CLEANUP
+])
+
# NOTE: This test case runs two ovn-controllers inside the same sandbox (hv1).
# Each controller uses a unique chassis name - hv1 and hv2 - and manage
# different bridges with different ports. This is why all 'as' commands below
--
2.39.2
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
